My Beef With RuboCop

Consider a hypothetical class that filters out unsafe HTML elements:

class SanitizingFilter < HTMLFilter
  def render_element(name, attrs, content)
    if safe?(name, attrs)
      super
    else
      nil
    end
  end
end

The default RuboCop settings would have us change it to this:

class SanitizingFilter < HTMLFilter
  def render_element(name, attrs, content)
    super if safe?(name, attrs)
  end
end

Let us feast on a three-course meal of opinion about why this is worse.

That nil Was Written For A Purpose

My first beef — the hors d’oeuvre, if you will — is that Style/EmptyElse wants us to change from an explicit nil return value to an implicit one. It says there is a “redundant else-clause”. I’ll tell you what’s redundant: the Style/EmptyElse cop.

This cop would prefer that the else clause did not exist, like this:

class SanitizingFilter < HTMLFilter
  def render_element(name, attrs, content)
    if safe?(name, attrs)
      super
    end
  end
end

In terms of behaviour at runtime, this is exactly the same. In terms of readability, it is not.

The explicit nil in the original implementation implies that the method is being called for its return value — that it’s probably functional, with inconsequential side effects.

Making the nil implicit says that the return value is not important. This method now reads like it’s implemented by avoiding side effects within super.

So which one is it? Is the filter supposed to work by returning nil, or does it work by avoiding the side effects in the super call? These are very different behaviours, and RuboCop just changed the human interpretation from the correct one to the wrong one.

Bad cop. No doughnut.

It’s Not A Guard Clause

My other beef (the main course) is with Style/GuardClause. This cop takes conditionals and turns one of the branches into a guard clause — something like this:

class SanitizingFilter < HTMLFilter
  def render_element(name, attrs, content)
    return nil unless safe?(name, attrs)

    super
  end
end

Again, the runtime behaviour is identical, but the cop makes it less readable.

Guard clauses are for bailing out early when you know that it’s not necessary to run the rest of the method. They are usually trivial in comparison to the real work done by the rest of the method. For example, if we were implementing a sorting algorithm, then we could have a guard clause for inputs with one element or less — there is no need to run a sorting algorithm when there is nothing to sort.

Except in this case, we’re not “guarding” the rest of the method from being run. That conditional is the entire raison d’être of the class. It’s the most important part. The whole purpose of a filter is to decide what elements stay in, and what elements get filtered out.

RuboCop has taken the most important part of the class and turned it into a trivial-looking guard clause.

Bad cop. No doughnut.

Branching Should Look Like Branching

My final qualm (is there such a thing as dessert beef?) is with Style/GuardClause removing the obvious indenting of the two branches, making it look like straight-line procedural code.

Most of the time, developers don’t so much read code as they do scan code. We don’t read from left to right, top to bottom, as if the codebase were the world’s most boring novel. We skim over code, navigating by the colours of syntax highlighting and the shapes made by indentation.

So when we’re skimming over the original code, trying to find the conditional — which we’ve already established as being the most important part of the class — we will find two landmarks: a line that starts with a brightly coloured if keyword, and indentation that suggests a conditional.

But skimming over the guard-clause-enhanced code we see neither of these landmarks. Instead, we find indentation that suggests procedural code: a series of statements that run from top to bottom. RuboCop has taken an obvious conditional and reshaped it to look like there is no conditional.

The Style/GuardClause cop is double bad, and misses out on two doughnuts.

Disclaimer/Conclusion

I don’t dislike RuboCop. It’s well made, and a useful tool.

The problem starts when it is viewed not as a tool, but as a set of divine commandments. Thou shalt not make implicit nils explicit. Thou shalt not write long-form conditionals. Thus saith the RuboCop.

This is not necessarily a problem with RuboCop itself — it’s a problem with how people sometimes use RuboCop. One could argue that the defaults aren’t great, but they are not a problem until someone hooks them up to CI.

My only aim here is to disabuse readers of the notion that RuboCop can only make code better. It’s a tool, and whether it helps or hurts depends on how it’s used. Don’t be afraid to disable cops if you can’t see how they benefit the team.