← Back to Upcase

Refactoring the controller method


(Charlieanna) #1

How can i refactor this method to follow the rule that the method should be no more than 5 lines.

 def create
    @user = User.find_by_email(params[:session][:email].downcase)
    if @user && @user.authenticate(params[:session][:password])
      @user.connect
      flash[:attacher] = attacher(@user)
      sign_in @user
      flash[:success] = 'Welcome back to IdleCampus!'
      redirect_to home_path
    else
      flash[:error] = 'Invalid email/password combination'
      redirect_to signin_path
    end
  end

(Zachary Davy) #2

I might extract some of the steps out into helper methods.


(Guirec Corbel) #3

Hello,

I think I will to something like this :

def create
  if sign_in_user
    sign_in_successful
  else
    sign_in_not_successful
  end
end

def user
   @user ||= User.find_by_email(params[:session][:email].downcase)
end

def sign_in_user
  return false if user.nil? || !user.authenticate(params[:session][:password])
  user.connect
  sign_in user
  true
end

def sign_in_succesful
  flash[:attacher] = attacher(user)
  flash[:success] = 'Welcome back to IdleCampus!'
  redirect_to home_path
end

def sign_in_not_succesful
  flash[:error] = 'Invalid email/password combination'
  redirect_to signin_path
end

It can be useful to do a class like this :

class Authenticator
  attr_accessor :user
  def initialize(user: user)
    @user = user
  end

  def authenticate
    return false if user.nil? || !user.authenticate(params[:session][:password])
    user.connect
    sign_in user
    true
  end
end

And call it like this :

def create
  if Authenticator.new(user: user).authenticate
    sign_in_successful
  else
    sign_in_not_successful
  end
end

What you think?

An additional question : Why do you need an authenticate method, a connect method and a sign_in method which seems to do the same things.