-1

I'm working a project where I found another developer wrote a method as you see it below. How would I clean those IF statements and refactor this method.

Also is it ok ti set a variable to nil ?

  def set_tenant
    if current_user
      if current_user.tenant_id.present?
        if current_user.tenants.include?(current_user.tenant)
          current_account = current_user.tenant
          set_current_tenant(current_account)
        else
          set_current_tenant(nil)
        end
      else
        set_current_tenant(nil)
      end
    else
      set_current_tenant(nil)
    end
  end

Thanks

egyamado
  • 117
  • 3

1 Answers1

1

Short Circuit Evaluation is your friend here. It means a false before an && will drop you to the else much like an if would have. It just leaves you with only one else instead of many. As an example here's a cleaner way to layout the code:

def set_tenant
  if current_user 
  && current_user.tenant_id.present? 
  && current_user.tenants.include?(current_user.tenant)
    current_account = current_user.tenant
    set_current_tenant(current_account)
  else
    set_current_tenant(nil)
  end
end

If prefer && over if chains in cases like this. It makes the code look simpler.

This layout shows the structure of the logic and I find it more readable than the stream of word wrapped noise you get if you jam all the &&s on the same line. Ruby doesn't care about whitespace so it works but your style guide might have something to say about it.

If it doesn't then look for examples in the existing code base and conform to that. Here are some examples of multi-line ruby if's from stack overflow.

And, as Thomas Owens points out, so long as you can think of a good name for the logic, this sets up Consolidate Conditional Expression. Which might look like this:

def set_tenant
  if tenant_included
    current_account = current_user.tenant
    set_current_tenant(current_account)
  else
    set_current_tenant(nil)
  end
end

def tenant_included
  return current_user 
    && current_user.tenant_id.present? 
    && current_user.tenants.include?(current_user.tenant)
end
candied_orange
  • 102,279
  • 24
  • 197
  • 315
  • 1
    This refactoring also opens the door to [Consolidate Conditional Expression](https://refactoring.com/catalog/consolidateConditionalExpression.html), where `current_user && current_user.tenant_id.present? && current_user.tenants.include?(current_user.tenant)` can be put into a method with a descriptive name for use in the conditional. – Thomas Owens May 04 '21 at 17:35
  • @ThomasOwens absolutely. If you have a good name for this logic that refactoring will give it a good home. – candied_orange May 04 '21 at 17:40
  • Thank you @candied_orange for the solution. It works well. I thought of using `&&` but didn't know how and where to user. Your answer shows me the way. Yes my style guide wasn't happy of having `&&` in a separate lines, so I merged them together on one long line. My next step is to refactor it more in it's own method as @ThomasOwens and @candied_orange suggested. – egyamado May 04 '21 at 18:13
  • 1
    @egyamado it's a pity. Old code has a lot of inertia. Remember, it's the team that decides on the style guide. And style guides can be changed if the team agrees. Don't ignore how it looks next to the old code. Looking better isn't wrong. Just don't confuse me. – candied_orange May 04 '21 at 18:22