Feature request: Checks that unless is not used with multiple conditions.

[wata727]

I was worried about the poor readability of code using unless with multiple conditions.

unless foo && bar
  something
end

Looking at the code like the above, I first think about De Morgan's law, rewrite it and proceed with implementation. This is a very painful task.

In order to prevent this problem, I created a cop called Lint/UnlessMultipleCondtions.
wata727@da4ed99

However, when this Cop is executed to RuboCop, the result is as follows:

...

lib/rubocop/config_loader.rb:128:23: W: Lint/UnlessMultipleConditions: Avoid using unless with multiple conditions.
        return unless hash['AllCops'] && hash['AllCops'][version]
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1101 files inspected, 140 offenses detected
RuboCop failed!

I don't mind fixing these problems, but I doubt if this fix will be accepted by the community.

Please tell me your opinion :)

[mikegee]

This seems like a good idea. 140 offenses in Rubocop's code is a bad sign though. Maybe it will be too noisy and should be disabled by default?

[asherkach]

Disagree, I think the unless foo && bar structure can be far more readable than if !foo && !bar depending on what foo and bar are. Granted it can be far less readable for the cases where it is natural to invoke De Morgan's Law. Banning the usage of conjunctions in unless statement seems like a bad idea to me.

[pocke]

It's a very nice cop!
BTW, I think the cop should be in the Style department instead of the Lint department. Because unless foo && bar style is not a bug, but the style confuses us and sometimes we make a bug with the style. And I think the cop should be enabled by default if it's in the Style department.

[mikegee]

Disagree, I think the unless foo && bar structure can be far more readable than if !foo && !bar

That is true. When I said I thought this cop was a good idea, I was thinking developers should extract these more complicated conditions to a method.

[wata727]

While looking the code where the offenses were found, I noticed one thing.
In an environment where Hash#dig can not be used, the following code is often used for checking elements of a nested hash:

return unless hash[:a] && hash[:a][:b]

I think that this code should not be fixed as follows:

return if !hash[:a] || !hash[:a][:b]

In such a case, I think that the first example can be used as a kind of idiom. So, I'm wondering whether this cop should be enabled by default...

[Drenmi]

cat.pet unless cat.lion? || cat.tiger?

cat.pet unless cat.large? && cat.fangs?

These examples both seem perfectly readable.

As per @mikegee's suggestion, one can implement Cat#dangerous?, but the question is if this generalizes across common examples. 🤔

[wata727]

These examples both seem perfectly readable.

I agree with it. I think that there are occasionally bad examples, but I understood that there are circumstances to use like this.

but the question is if this generalizes across common examples. 🤔

In order to avoid this Cop, I think that rewriting of understandable code should be avoided.

Based on these, I'm thinking whether to take either of the following:

1. Add this Cop with Enabled: false
2. Close this issue and the pull request

wdyt?

[wata727]

I will close this issue once. Thank you very much for giving us your opinion.

[Morozzzko]

I've been hoping to see this cop for a long time. It's a shame it wasn't merged.

Have you had any chance to revisit this issue and, perhaps, think about better impementation?

While I agree that not all composite conditions are bad, especially in the multiline approach, I'd love to have an option to warn about conditions that look return xxx unless a && b || c

There's rarely a good reason to use such complex examples, to be honest.

I've found that I write this code for two reasons:

Compaction. If I have multiple guards, I can turn them into one

return unless user
return unless order.shipped?

# turns into

return unless user && order.shipped?

I still believe that this code is harder to reason about than the alternative. While the compaction may be good, it's still not as good as the composite if

return if user.nil? || !order.shipped?

or even better – some kind of predicate that encapsulates the logic

return unless applicable?

Avoiding explicit #nil? checks. While it will definitely trick reek into not firing NilCheck violation, it's still not a decent reason to make the code more complicated

While I believe that this cop should not be auto correctable, it should probably exist in some way

[MikeMcQuaid]

I've been hoping to see this cop for a long time. It's a shame it wasn't merged.

Agreed!

cat.pet unless cat.lion? || cat.tiger?

cat.pet unless cat.large? && cat.fangs?

These examples both seem perfectly readable.

Just to offer my own $0.02: I've been working with Ruby professionally for ~8 years, through being a Homebrew maintainer for ~12 years and programming professionally (not just using Ruby) for ~14 years and: I find all cases of unless ... || or unless ... && (including the above) hard to mentally parse and, particularly, to accurately and consistent reverse the logic (by turning it into an if).

That's not to say it should be a default cop or whether it should be a style/lint/whatever cop but: I think there is a valid need for a cop like this and it can improve readability (particularly for those coming from languages without an unless operator).

[Drenmi]

It seems there's good reason to think this cop would be useful to people, and the issue was only automatically closed by GitHub when the first PR was closed by it's author, so let's re-open this one. 🙂

[caalberts]

While I agree that not all composite conditions are bad, especially in the multiline approach, I'd love to have an option to warn about conditions that look return xxx unless a && b || c

This is a good idea. The starting point could be to warn against using opposing boolean operators in unless.

# ok
return xxx unless a && b && c
return xxx unless a || b || c

# not ok
return xxx unless a && b || c ...
return xxx unless a || b && c ...

What should it do if the condition is within (), which would improve readability significantly?

return xxx unless a && (b || c)

[caalberts]

I created #9386 to check for mixed use of && and || in unless. I think this is a more lenient approach as compared to checking unless with multiple conditions and would be more palatable. Currently there are 6 offenses in Rubocop.