user3437721 February 2016

ruby - refactoring if else statement

I've tried reading some tutorials on refactoring and I am struggling with conditionals. I don't want to use a ternary operator but maybe this should be extracted in a method? Or is there a smart way to use map?

detail.stated = if value[:stated].blank?
                  nil
                elsif value[:stated] == "Incomplete"
                  nil
                elsif value[:is_ratio] == "true"
                  value[:stated] == "true"
                else
                  apply_currency_increment_for_save(value[:stated])
                end

Answers


Ilya February 2016

 detail.stated = if value[:stated].blank? || value[:stated] == "Incomplete"
                   nil
                 elsif value[:is_ratio] == "true"
                   value[:stated] == "true"
                 else
                   apply_currency_increment_for_save(value[:stated])
                 end


DigitalRoss February 2016

stated = value[:stated]
detail.stated = case
  when stated.blank? || stated == "Incomplete"
    nil
  when value[:is_ratio] == "true"
    value[:stated] == "true"
  else
    apply_currency_increment_for_save stated
end

What's happening:   when case is used without an expression, it becomes the civilized equivalent of an if ... elsif ... else ... fi.

You can use its result, too, just like with if...end.


dgmora February 2016

Move the code into apply_currency_increment_for_save and do:

def apply_currency_increment_for_save(value)
  return if value.nil? || value == "Incomplete"
  return "true" if value == "true"
  # rest of the code. Or move into another function if its too complex
end                         

The logic is encapsulated and it takes 2 lines only


Jordan February 2016

If you move this logic into a method, it can be made a lot cleaner thanks to early return (and keyword arguments):

def stated?(stated:, is_ratio: nil, **)
  return if stated.blank? || stated == "Incomplete"
  return stated == "true" if is_ratio == "true"
  apply_currency_increment_for_save(stated)
end

Then...

detail.stated = stated?(value)


AndyV February 2016

I like @Jordan's suggestion. However, it seems the call is incomplete -- the 'is_ratio' parameter is also selected from value but not supplied.

Just for the sake of argument I'll suggest that you could go one step further and provide a class that is very narrowly focused on evaluating a "stated" value. This might seem extreme but it fits with the notion of single responsibility (the responsibility is evaluating "value" for stated -- while the 'detail' object might be focused on something else and merely makes use of the evaluation).

It'd look something like this:

class StatedEvaluator
  attr_reader :value, :is_ratio

  def initialize(value = {})
    @value = ActiveSupport::StringInquirer.new(value.fetch(:stated, ''))
    @is_ratio = ActiveSupport::StringInquirer.new(value.fetch(:is_ratio, ''))
  end

  def stated
    return nil if value.blank? || value.Incomplete?
    return value.true? if is_ratio.true?
    apply_currency_increment_for_save(value)
  end
end

detail.stated = StatedEvaluator.new(value).stated

Note that this makes use of Rails' StringInquirer class.

Post Status

Asked in February 2016
Viewed 2,214 times
Voted 10
Answered 5 times

Search




Leave an answer