Rubocop: GuardClause complaining when single line is not gonna work

[aliismayilov]

Expected behavior

No complains

Actual behavior

Style/GuardClause cop is complaining.

Steps to reproduce the problem

here's a code snippet:

if object.property.condition?
  object.metadata = {
    yes_parameter: object_params[:no_parameter].nil?
  }
end

rubocop will recommend to use a guard clause, in other words, the following snippet:

object.metadata = {
  yes_parameter: object_params[:no_parameter].nil?
} if object.property.condition?

but then i'd have Style/MultilineIfModifier complaining. And if i'd try to write it in one line, then the line would be too long.

RuboCop version

$ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.3.3 x86_64-darwin16)

[tjwp]

Is that the entire body of a method? My guess is the cop is actually trying to get you to write:

return unless object.property.condition?
object.metadata = {
  yes_parameter: object_params[:no_parameter].nil?
}

I.e. remove a level of nesting if that is the entirety of the method.

I had the same confusion yesterday. Perhaps the message needs to be improved to better explain the change that is suggested.

[aliismayilov]

yep, you are right, it's happening only at the bottom of the method.

[Drenmi]

Is there some way we could improve the message for this cop, to clarify what it's actually suggesting? 🙂

[michniewicz]

I would suggest smth like:
Instead of wrapping the code inside a conditional expression use a guard clause above it
since the guard clause is a conditional statement that should be placed _above_ the particular piece of code or at the top of the function/block.

WDYT?

[Drenmi]

Seems like a reasonable message. It is important to point out that this message only applies when:

1. The resulting line would be too long.
2. The conditional is the last part of it's definition.

Consider this example with additional lines below:

if foo
  return bar
end

baz

Here we could not change to:

return unless foo

bar
baz

These two conditions are already baked into the logic of this cop, but it seemingly doesn't adjust the message depending on the path it takes.

If anyone is keen to open a PR to improve it, feel free. 🙂