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.
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
value to an implicit one. It says there is a “redundant
else-clause”. I’ll tell you what’s redundant: the
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.
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.
nil implicit says that the return value is not
important. This method now reads like it’s implemented by avoiding
side effects within
So which one is it? Is the filter supposed to work by returning
or does it work by avoiding the side effects in the
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.
Style/GuardClause cop is double bad, and misses out on two
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
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.