← Back to Upcase

Is this a bad practice?


(Scott Hollinshead) #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


(Ben Orenstein) #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

(Scott Hollinshead) #3

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

http://forum.thoughtbot.com/t/how-could-i-make-this-code-better/1030/17


(Ben Orenstein) #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

(Scott Hollinshead) #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

(Ben Orenstein) #6

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

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


(Scott Hollinshead) #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.


(George Ulmer) #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