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
@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
@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: 7 Patterns to Refactor Fat ActiveRecord Models | Code Climate (see point NĀŗ 2 - Extracting Service Objects).
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.
@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.
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.
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
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.
