1
\$\begingroup\$

Would love some feedback on this simple API implementation with Ruby on Rails. Have I used proper naming conventions, readable/manageable code, optimal approaches, etc? As I'm still learning Ruby on rails, I'd love to hear what other fellow engineers have to say about this :)

A simple Register API:

require 'net/http'
require 'uri'
require 'json'

class V1::UsersController < ApplicationController
    protect_from_forgery with: :null_session

    def register
        name = params[:name]
        email = params[:email]
        password = params[:password]

        @existing_user = User.find_by(email: params[:email])

        if @existing_user == nil
            @user = User.new(
                name: name,
                email: email,
                password: password,
                plan: "FREE"
            )
            #save user
            if @user.save!
                #generate auth token
                @auth = save_new_auth @user.id

                render :json => {
                    :user => @user.as_json(:except => [:created_at, :updated_at, :password, :stripe_id, :subscription_id, :id, :email]),
                    :auth => @auth
                }
            else
                render json: {"error" => "Unprocessable Entity"}
            end
        else
            render json: { error: { type: "UNAUTHORIZED", message: "Looks like you already have an account. Please login 👋" } }, status: 403
        end
    end

Login API:

def login
    email = params[:email]
    password = params[:password]
    
    @auth = nil
    
    @user = User.find_by(email: params[:email], password: password)
    
    if @user == nil
        render json: { error: { type: "UNAUTHORIZED", message: "Invalid Login Credentials 🤔" } }, status: 401
    else
        @auth = UserAuth.find_by(user_id: @user.id)
        if @auth == nil
            @auth = save_new_auth @user.id
        end
        render :json => {
            :user => @user.as_json(:except => [:created_at, :updated_at, :password, :stripe_id, :subscription_id, :id, :email]),
            :auth => @auth
        }
    end
end

Access token Generator:

def save_new_auth (user_id)
    @auth = UserAuth.new(
        access_token: generate_token,
        user_id: user_id,
        status: true
    )
    @auth.save
        return @auth
    end
    
def generate_token(size = 28)
    charset = %w{ 2 3 4 6 7 9 A C D E F G H J K M N P Q R T V W X Y Z}
    (0...size).map{ charset.to_a[rand(charset.size)] }.join
end
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Here are a couple of suggestions

Non default CRUD action in Controller

It's generally recommended to only have CRUD actions index, show, new, edit, create, update, destroy in your controllers. So e.g. you should think about renaming your register and login actions and extract according controllers.

For example you could have a UserRegistrationsController and a UserLoginsController which each have a create action.

class V1::UserRegistrationsController < ApplicationController
  def create
  end
end

class V1::UserLoginsController < ApplicationController
  def create
  end
end

Here is a good blog article explaining this concept http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/

Handling user not found

Instead of using find_by you could use the bang method find_by! which will raise a RecordNotFound error.

User.find_by!(email: params[:email])

Now RecordNotFound errors will get automatically translated to a 404 response code. But you could overwrite the default behaviour as well with your own exception app or rescue_from

https://github.com/rails/rails/blob/main/actionpack/lib/action_dispatch/middleware/public_exceptions.rb#L14 https://stackoverflow.com/questions/62843373/how-to-deal-with-general-errors-in-rails-api/62846970#62846970

Another possibility is to add a validation with scope so you can only have one email address.

class V1::UserRegistrationsController < ApplicationController
  def create
    @user = User.new(
      name: name,
      email: email,
      password: password,
      plan: "FREE"
    )

    if @user.save
      render @user
    else
      render @user.errors
    end
  end

Strong parameters

User strong parameters instead of plucking the values from the params hash.

der user_params
  params.require(:user).permit(:email, :password, :name)
end

https://edgeguides.rubyonrails.org/action_controller_overview.html#strong-parameters

Use a PORO or Service object

A lot of people will have different opinions of using a Plain Old Ruby Object vs a Service object etc. But I think a lot of people would agree that it's a good idea to move logic from the controller to a dedicated object.

Example

class UserRegistration
  def self.create(params)
    new(params).save
  end

  def initialize(params)
    @params = params
  end

  def save
    user = create_user
    auth = UserAuth.new(
        access_token: generate_token,
        user_id: user.id,
        status: true
    )
    auth if auth.save
  end

  private

  attr_reader :params

  def create_user
    User.create(
      name: params[:name],
      email: params[:email],
      password: params[:password],
      plan: "FREE"
    )
  end

  def generate_token(size = 28)
    charset = %w{ 2 3 4 6 7 9 A C D E F G H J K M N P Q R T V W X Y Z}
    (0...size).map{ charset.to_a[rand(charset.size)] }.join
  end
end

And then you can just do in your controller

class V1::UserRegistrationsController < ApplicationController
  def create
    if (auth = UserRegistration.create(params))
      render auth
    else
      render :error
    end
  end
end

Generally speaking I think there is too much logic in your controllers which makes it hard to read and test.

\$\endgroup\$
3
  • \$\begingroup\$ This is very useful. Thank you so much for taking the time Christian! I appreciate it! :) \$\endgroup\$ Commented Nov 1, 2021 at 11:45
  • \$\begingroup\$ If you like the answer, please consider to accept it. \$\endgroup\$ Commented Nov 1, 2021 at 22:46
  • \$\begingroup\$ @ChristianBruckmayer when talking about using a PORO v Service object, is the idea to create a new controller anytime I need code to do something other than the basic CRUD actions, so the object adheres to basic CRUD actions? I guess i'm a little confused about your class UserRegistration; Ideally that's in a controller? \$\endgroup\$ Commented Feb 2, 2022 at 16:33

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.