Refactory scenario: removing conditional blocks and extracting code to a private method

This is a simple but very useful refactory strategy.

It allows you to easily make the code more readable and clear by having a method name that explains what’s going on and also avoiding blocks of conditionals by turning them into guard clauses.

Let’s use real code from a Rails action as an example, but this can be applied in any programming language:

# users/registrations#new
def new
  email = params[:email].strip.downcase
  @lead = Lead.find_or_initialize_by(email: email)

  unless @lead.persisted?
    valid_email_format = @lead.email.match(URI::MailTo::EMAIL_REGEXP).present?

    if valid_email_format && EmailVerificationService.new.valid?(email)
      @lead.valid = true
    end

    if @lead.valid
      clearbit_data = Clearbit.new.email_lookup(email)
      name_hash = clearbit_data.fetch('person', {}).fetch('name', {})

      if name_hash.present?
        first_name = name_hash.fetch('givenName', nil)
        if first_name.present? && first_name != 'Name'
          @lead.first_name = first_name
        end

        last_name = name_hash.fetch('familyName', nil)
        if last_name.present? && last_name != 'Name'
          @lead.last_name = last_name
        end
      end
    end

    @lead.save! if @lead.changed?
  end
end

Whaaaat?! Why is this action so long? What’s going on here!?

Yup that would also be my reaction seeing that amount of code in an action 😆

Even though it’s on a Users::RegistrationController, it’s doing many things with the Lead model. Yup, that’s how it is on real-life projects. 😉

However, should the programmer have to read every single line of code to grasp what’s going on there? Nope!

Extracting related logic to a private method

So the first thing I like to do is extract all the code that’s related to the same thing to a private method:

def new
  email = params[:email].strip.downcase
  @lead = Lead.find_or_initialize_by(email: email)

  personalize_the_signup_view
end

private

def personalize_the_signup_view
  unless @lead.persisted?
    valid_email_format = @lead.email.match(URI::MailTo::EMAIL_REGEXP).present?

    if valid_email_format && EmailVerificationService.new.valid?(email)
      @lead.valid = true
    end

    if @lead.valid
      clearbit_data = Clearbit.new.email_lookup(email)
      name_hash = clearbit_data.fetch('person', {}).fetch('name', {})

      if name_hash.present?
        first_name = name_hash.fetch('givenName', nil)
        if first_name.present? && first_name != 'Name'
          @lead.first_name = first_name
        end

        last_name = name_hash.fetch('familyName', nil)
        if last_name.present? && last_name != 'Name'
          @lead.last_name = last_name
        end
      end
    end

    @lead.save! if @lead.changed?
  end
end

Ok I already see a significant improvement, if the developer reading this code isn’t interested in code related to personalization of the view for the Lead, he can stop there 👍

Removing conditional blocks

So now we can get rid of some conditional blocks by turning them into guard clauses:

def personalize_the_signup_view
  return if @lead.persisted?

  valid_email_format = @lead.email.match(URI::MailTo::EMAIL_REGEXP).present?

  if valid_email_format && EmailVerificationService.new.valid?(email)
    @lead.valid = true
  end

  return unless @lead.valid

  clearbit_data = Clearbit.new.email_lookup(email)
  name_hash = clearbit_data.fetch('person', {}).fetch('name', {})

  return if name_hash.blank?
  
  first_name = name_hash.fetch('givenName', nil)
  if first_name.present? && first_name != 'Name'
    @lead.first_name = first_name
  end

  last_name = name_hash.fetch('familyName', nil)
  if last_name.present? && last_name != 'Name'
    @lead.last_name = last_name
  end

  @lead.save! if @lead.changed?
end

I find it easier to understand what’s going on. You can see that if the Lead is already in the database, we stop there. If the email has an invalid format, we won’t even bother calling Clearbit API, and we also return early if Clearbit doesn’t return the kind of data we are expecting.

Yeah, I know there are other ways to optimize this code even more, but the goal of this post is to show only “one thing”. One example, so it’s easier to understand.

1 Comment

Leave a Comment