-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

0

1 Answer 1

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
4
  • 1
    This refactoring also opens the door to Consolidate Conditional Expression, 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. Commented May 4, 2021 at 17:35
  • @ThomasOwens absolutely. If you have a good name for this logic that refactoring will give it a good home. Commented May 4, 2021 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. Commented May 4, 2021 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. Commented May 4, 2021 at 18:22

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.