← Back to Upcase

How could I make this code better?


(Scott Hollinshead) #1

How could I clean this action up?

def create
    user = User.find_by_email(params[:email])
    if user && user.authenticate(params[:password]) && user.verified
      session[:user_id] = user.id
      flash.notice = "Successfully Signed In!"
      redirect_to dashboard_path
    elsif user && !user.verified
      flash.now.alert = "Please Verify your account before logging in"
      render "new"
    else
      flash.now.alert =  "Invalid Username or Password!"
      render "new"
    end
  end

I think this looks a little messy

Thanks


(Derek Prior) #3

@scott, what is this a create action for? A session?

I think your code is more complicated than it needs to be because authenticate is implemented as an instance method on User. It’s important that User have credentials, but I don’t think that it needs to know how to perform that authentication. What if authentication were the responsibility of a controller method?

def authenticate
  User.find_by_email_and_password(params[:email], params[:password])
end

now your create looks like this:

def create
  if user
    if user.verified
      # sign them in
    else
      # tell them to verify
    end
  else
    # tell them they are invalid
  end
end

def user
  @user || authenticate
end

That’s still not great, but we’re getting somewhere. You could extract the lines that set flash.now and render 'new' into a method that takes the error message. That would eliminate some duplication. As you start cleaning it up, you might see more possible refactorings - maybe extracting a before filter to handle some of the error cases?

The key is to take small steps, run your tests to verify, and see how things feel to you.

As an aside, it appears your authentication scheme here is relying on plaintext passwords being stored in the database. If this code is anything more than an exercise I would probably move to something like Clearance for authentication as it worries about such things so you don’t have to :wink:


(Pedro Moreira) #4

@scott, like we discussed in the Learn Campfire room, here’s my thoughts on this:

If this will be in production, I would suggest you use a gem like Clearance or Devise and not trying to implement an authentication system yourself.

Second, if you just want to learn about authentication systems, I would recommend reading the source code for Clearance or Devise: fork the repos, run the tests, poke around in the code. If you take this option, I’d recommend studying Clearance first. Devise is pretty extensive and may bring more confusion than enlightenment initially.

Lastly, if this is really just for practice, I would probably try to move stuff over to a class that handles this.
Here is an example from the Code Climate Blog:
http://blog.codeclimate.com/blog/2012/10/17/7-ways-to-decompose-fat-activerecord-models/ (see point Nº 2 - Extracting Service Objects).


(Chad Pytel) #5

I don’t want to pile on, but I can’t stress enough that you should not be writing an authentication system from scratch for use in a production system.


(Scott Hollinshead) #6

@derekprior It is a create for a session yes sorry. I am using bcrypt so the passwords are being hashed and salted into the db and not stored in plain text.
The authenticate method is also part of bcrypt, this is the code if you want to have a better look https://github.com/scott2619/aas/tree/products_model.

@pedromoreira Thanks I will have a look.

@cpytel I am doing this as an exercise so I can see how things are working.


(Chad Pytel) #7

@scott Great, I’m glad to hear its not for production.


(Scott Hollinshead) #8

@cpytel How would you make the create method above better?


(George Brocklehurst) #9

Building on what @derekprior wrote, I think you could also flatten out the nested conditionals by changing the order of the clauses, which would make things a bit more readable:

def create
  if user.nil?
    # tell them they are invalid
  elsif user.verified
    # sign them in
  else
    # tell them to verify
  end
end

A few other things you might want to consider:

  • Ending methods that return a boolean with a ?, for example user.verified? instead of user.verified (if verified is a database field on an ActiveRecord model then you’ll already have a verified? method).
  • Extracting each branch of the conditional as a private method, so I don’t have to read and understand all of the details of each branch to understand the create method.

(Scott Hollinshead) #10

@georgebrock Thanks in @derekprior’s user method he does

however authenticate is a method from BCrypt so could I do this? or is there a better way?

def user(user)
  @user ||= user.authenticate(params[:password]) if user
end

Thanks


(Scott Hollinshead) #11

I have now come up with this, have I made it better or worse?

If worse how could I improve?

class SessionsController < ApplicationController
  def new
    redirect_to companies_path if current_user
  end

  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

  def destroy
    session[:user_id] = nil
    redirect_to root_url, notice: "Logged out!"
  end

  private

  def user
    @user || authenticate
  end

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

  def sign_in(user)
    session[:user_id] = user.id
    flash.notice = "Successfully signed in!"
    redirect_to companies_path
  end

  def failed_login(message)
    flash.now.alert = message
    render "new"
  end

end


(Chad Pytel) #12

Can you please edit your comment to provide the whole controller, or at least any before filters?


(Scott Hollinshead) #13

I have edited the post :slight_smile:


(Chad Pytel) #14

I don’t understand how @user would be set in def user, is that correct?


(Scott Hollinshead) #15

I don’t fully understand the user method myself, It was what @derekprior suggested.

I thought it was to catch the nil case of user.nil ?

Should it be @user ||= authenticate ?


(Derek Prior) #16

Yes, ||= is what I meant. It is acting as an accessor for the user variable but also performing the authentication the first time its called. It’s a little awkward because it’s quite possible authenticate will return nil, in which case calling user again will cause authenticate to run again, etc, etc. In this use case it works, though, because the very first thing you do with user is handle the case where its nil.


(Scott Hollinshead) #17

Thank you for clearing that up for me @derekprior