CODEWITHSUNDEEP
rubycoding-stylerubocop
Jeff Nyman
Jeff Nyman

Reputation: 890

Refactoring More Than Two Levels of Block Nesting for Rubocop

I am working to get better at my Ruby code, particularly in terms of styling it per Ruby idioms. I have the following method:

def self.carnivore_convert
  lambda do |value, field|
    diet_type = value
    if field[:header].to_s == 'diet'
      diet_value = value.to_s.downcase
      if diet_value =~ /yes|no/
        diet_value == 'yes' ? diet_type = 'Carnivore' : diet_type = ''
      end
    end
    diet_type
  end
end

Rubocop complains about this, saying that I should:

"Avoid more than 2 levels of block nesting."

It's specifically referring to this line:

diet_value == 'yes' ? diet_type = 'Carnivore' : diet_type = ''

However, I'm not clear at all as to why this is a "style violation" and anything I do to "fix" this seems to make the code a bit more messy, at least in my view.

When I say I don't get why it's a style violation, I do understand that Rubocop apparently feels I should not have an if...if...if sort construct. But I'm not sure why that's so wrong.

I was thinking I could perhaps get rid of my first if condition in this code by using a guard clause of some sort but I can't do that because then my code doesn't work. For example, I changed my code like this:

def self.carnivore_convert
  lambda do |value, field|
    diet_type = value
    next unless field[:header].to_s == 'diet'
    diet_value = value.to_s.downcase
    if diet_value =~ /yes|no/
      diet_value == 'yes' ? diet_type = 'Carnivore' : diet_type = ''
    end
    diet_type
  end
end

But, while not erroring, this does make the code not work as intended. I tried break and return instead of next as well.

So I'm not sure how to get this code into a shape that Rubocop will not complain about and still have it be readable. As a side issue to that, I feel like I'm spending a lot of time on "Ruby styling" even when I have code that definitely works. As long as I can understand why the style is a violation I don't mind, but here I'm having trouble seeing it. That being said, I've certainly learned that I have long way to go towards building my intuition for this stuff.

Regarding the above issue, does anyone have any ideas?

Upvotes: 3

Views: 4967

Answers (1)

ndnenkov
ndnenkov

Reputation: 36110

If you get a nesting warning this basically means "there is too much going on here". Sometimes there are other problems with the style and you can fix them (like guard clauses for example). Other times you just have to split the code. The smaller your methods and classes are, the more readable and likely to have single responsibility. The code below is far from perfect, but it just aims to show a simple change to make it more readable and follow constraints:

def self.carnivore_convert
  lambda do |value, field|
    field[:header].to_s == 'diet' ? diet_type(value) || value : value
  end
end

def self.diet_type(diet_value)
  diet_value = diet_value.to_s.downcase
  if diet_value =~ /yes|no/
    diet_value == 'yes' ? 'Carnivore' : ''
  end
end
private_class_method :diet_type

EDIT: Here is another version that is equivalent to the original code, but is easier to grasp:

def self.carnivore_convert
  lambda do |value, field|
    simplified_value = value.to_s.downcase

    if diet_header?(field) && simplified_value == 'yes'
      'Carnivore'
    elsif diet_header?(field) && simplified_value =~ /yes|no/
      ''
    else
      value
    end
  end
end

def self.diet_header?(field)
  field[:header].to_s == 'diet'
end
private_class_method :diet_header?

Upvotes: 4

Related Questions