Refactoring multiple if statements for user authentication with subdomains

I'm building a typical web app where once a user signs up they access the app through their own subdomain (company.myapp.com). The "checking what kind of user if any is logged in" piece is starting to get very hairy and it obviously needs to be well-written because its run so often so I was wondering how you guys would re-factor this stuff.

Here are the different states:

1. A user must be logged in, t开发者_如何学JAVAhe user must not have a company name, and the sub-domain must be blank
2. A user must be logged in, the user must have a company name, that company name must match the current sub-domain
3. A user must be logged in, the user must have a company name, that company name must match the current sub-domain, and the user's is_admin boolean is true

if !session[:user_id].nil?
  @user = User.find(session[:user_id])
  if @user.company.nil? && request.subdomains.first.nil?
    return "state1"
  elsif !@user.company.nil?
    if @user.company.downcase == request.subdomains.first.downcase && !@user.is_admin
      return "state2"
    elsif @user.company.downcase == request.subdomains.first.downcase && @user.is_admin
      return "state3"
    end
  end
end

---

As Andrew said, you need to have this logic in your user model. I will go a step further and make each condition a method to keep the code clean, understandable, reausable and follow Single Responsibility Principle. And also define a method in the user model that returns state, like so.

class User

  def get_state(request)
    return 'state1' if no_company_and_subdomain?(request)
    return 'state2' if has_company_that_match_subdomain?(request)
    return 'state3' if admin_has_company_that_match_subdomain?(request)
  end

  def no_company_and_subdomain?(request)
    company.nil? && request.subdomains.empty?
  end

  def has_company_that_match_subdomain?(request)
    return false if company.nil? || is_admin
    company.downcase == request.subdomains.first.downcase
  end

  def admin_has_company_that_match_subdomain?(request)
    return false if company.nil? || !is_admin
    company.downcase == request.subdomains.first.downcase
  end

end

and then your controller code will become like this

if !session[:user_id].nil?
  @user = User.find(session[:user_id])
  @user.get_state
end

Obviously, you can rename the methods more sensibly based on your requirements.

---

Can you move some of the logic to the user model, rather than having it in the controller? That way, your code will be more DRY and better encapsulated.

---

Check out replace type with polymorphism and replace type with strategy refactoring pattern (in your case latter one seems to fit better). It might simplify your code, especially if you have multiple similar conditionals in your code.

Refactoring Ruby is a good source of informations on that topic.