Is this a bad practice?

Ruby on Rails
1

I have this authenticate method.

def authenticate
  user = User.find_by_email(params[:email])
  if user && user.authenticate(params[:password])
    user
  else
    nil
  end
end

I read the Sandi Metz Rules and she says to make methods no longer than 5 lines, so to make this method less than 5 lines could I do this or is it bad practice?

def authenticate
  user = User.find_by_email(params[:email])
  user if user && user.authenticate(params[:password])
end

Thanks

2
  1. Remember that Sandi’s “rules” are more like suggestions. If the code looks worse after you follow the rules than before, don’t just follow them blindly.
  2. I think a better refactoring looks like this:
def authenticate
  user && user.authenticate(params[:password])
end

private

def user
  User.find_by_email(params[:email])
end
3

@benorenstein thanks I need to have authenticate return user or nil for my user method, this is what it currently looks like.

4

The solution I showed does return either nil or user (assuming user.authenticate(params[:password]) returns a user.

Example:

>> nil && :foo
=> nil
>> :foo && :bar
=> :bar
5

@benorenstein It is where it finds a user and the user puts in the wrong password that it returns false therefor making my user method(below) return false.

so when my create method trys to do user.verified? it errors as the method does not exist for a False class.

So if it is okay to leave it the way it is I think this is the best way at the moment?

def user 
  @user ||= authenticate
end
6

But…verified isn’t defined on nil either, right?

I feel like I’m missing some code (or just misunderstanding you).

7

@benorenstein I linked you the code from a different post I made but here is the create method.

def create
  if user.nil?
    failed_login("Invalid email address or password!")
  elsif user.verified?
    sign_in(user)
  else
    failed_login("Please verify your account before logging in")
  end
end

here is the other post

The code will never get to verified? if the user is nil which is why I am returning nil if the user fails authentication.

8

I don’t know if this does what you want, but you may prefer this style of coding. elsif is pretty ugly.

def create
  failed_login("Invalid email address or password!") and return if user.nil?
  
  sign_in(user) and return if user.verified?

  failed_login("Please verify your account before logging in")
end