User registration API

Question

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

Answer 1

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.

• Jay: This is very useful. Thank you so much for taking the time Christian! I appreciate it! :)
• Christian Bruckmayer: If you like the answer, please consider to accept it.
• J.R. Bob Dobbs: @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?