Refactoring Conditionals for More Readable Code

#webdev #ruby #rails #refactoring

The today I was writing some code and it just felt weird when I started to write this long conditional. I asked a coworker of mine did he have any ideas to what we could do to make this more readable. We did the following

Before

  class GetStuff

    def fetch(current_url)
      response = handle_response(query).try(:first)
      return nil if !enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url

      response
    end

    private

    def enabled?(value)
      return false if value.nil?
      value.enabled
    end

    def postable?(value)
      Time.at(value.post_date) < Time.now && value.enabled
    end
  end

After


  class GetStuff

    def fetch(current_url)
      @current_url = current_url
      @response = handle_response(query).try(:first)

      return nil unless show?

      @response
    end

    private

    def show?
      return false if @response.nil?
      enabled? && postable? && is_current_page?
    end

    def is_current_page?
      false if @current_url == @response.global_banner.first.button_url
    end

    def enabled?
      @response.enabled
    end

    def postable?
      Time.at(@response.post_date) < Time.now && @response.enabled
    end
  end

We did a couple of things here we removed some reverse logic here.

!enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url

We don't need the ! anymore. I don't know about you but the less I have to think about if this thing is true reverse it the better.

We also simplified the variables being passed through to methods by making some instance variables.

Before

def fetch(current_url)
 response = handle_response(query).try(:first)
 return nil if !enabled?(response) && !postable?(response) && current_url == 
  response.global_banner.first.button_url

 response
end

After

def fetch(current_url)
 @current_url = current_url
 @response = handle_response(query).try(:first)

 return nil unless show?

 @response
end

We have a gaurd clause that is protecting us against a empty response from a server. We already had this but it wasn't really clear what it was doing and that it was protecting everything.

Before

return nil if !enabled?(response) && !postable?(response) && current_url == response.global_banner.first.button_url

def enabled?(value)
  return false if value.nil?
  value.enabled
end

After

def show?
  return false if @response.nil?
  enabled? && postable? && is_current_page?
end

Things are more readable and the methods explain why we have the checks in place and what is this condional doing. I feel like this is self documenting code.

Top comments (3)

Lud • Sep 29 '19 • Edited

Hey,

cleaning up code is always a good idea! Long conditional chains are indeed horrible to read and cost a lot of brainpower to process. Some thoughts on your refactoring:

    def fetch(current_url)
      @current_url = current_url
      response = handle_response(query).try(:first)

      response if show?
    end

is the same as

    def fetch(current_url)
      @current_url = current_url
      @response = handle_response(query).try(:first)

      return nil unless show?

      @response
    end

if you prefer it a bit shorter.

2) return nil unless show? is the same as return unless show?, there is no need to explicitly state the nil, a blank return always returns nil

3)handle_response(query).try(:first) will only work if you use the rails gem, because try is from rails. Instead, you can use the Ruby safe operator &: handle_response(query)&.first which removes your dependency on rails for this service.

4) would

    def is_current_page?
      @current_url != @response.global_banner.first.button_url
    end

be the same as

    def is_current_page?
      false if @current_url == @response.global_banner.first.button_url
    end

but just without one method call (if) less? Also, this method now will return nil if your if statement returns false, which seems strange for a method ending in a ?, because for these methods I expect a boolean response.

5) The naming seems inconsistent, you have enabled? but then is_current_page? - so I would either change your other methods to also start with is_ or remove the is_. I personally prefer to not start method names with is_ since it is not really necessary for readability in my opinion (English is not my native language, so my feeling for the english language might be wrong).