Unban and and or control flow operators

[zverok]

And so, after several years of private discussions, here I am!

My reasoning goes as this:

1. I remember/understand where the current state of the rule came from. Initially, and/or were understood by most of the community (and previous versions of this guide) as "broken logical operators" with surprisingly low priority, and, to add insult to injury, equal priority towards each other (unlike usual higher priority of logical "and"). But, as time went and approaches to Ruby style and coding practices became crystallized, it became obvious that and/or were designed for control flow, and their design is extremely suitable for this task, as shown by examples below:

# parsing and evaluation priority allows to use and/or as clear precondition for methods:
value = fetch_necessary_value or raise Error, "Pretty long error message"
value = fetch_optional_value or return

# their different spelling and priority are good for mixing them with logical evaluation
possible_error_check || other_possible_error && condition_for_that_error and raise
# There is **no** clear and DRY replacements for examples above.

2. This style guide went a long road since its inception. In current state, it is de-facto "The Style Guide", one that novices are immediately exposed to, one that The Linter (aka Rubocop) implements with default settings. With this in mind, stakes for "doing right" in style guide are much higher than their were yeas ago, it is a lot of responsibility for entire community/infrastructure. "Banning" of useful, clearly designed language feature with readable usages and obvious meaning seems just plain wrong—despite possible discussions of who prefers what.
3. It is important to observe that the style guide does NOT, in current state, totally against flow control with "and"/"or", but advices (inappropriately) && and || for it, which do not allow most of the good usages of flow control! It is slightly like "screwdrivers are banned; use your pocket knife to screw things".
4. Aside observation: no other language feature is "banned". Some are recommended to "avoid", most of the time with explanantion why and/or recommendations what should be used instead; the esoteric flip-flops are almost the only thing that just commented with "avoid, period", but even for recommendation is softened with "unless you know exactly why". So, this "personal hostility" towards this particular feature looks obviously outdated.
5. The last objection to and/or that could be thought of is their equal priority. I believe that it doesn't matter at all, as for the flow control more than one flow-control operator in a row is weird anyways (and my proposed change advices on that). As a one more side note, I assume that idea of equal priority came exactly from "flow control" domain: to avoid "reordering" of flow statements, in examples like: output or log and exit ("regular" priorities would read it as output or (log and exit), so no exit if output was successful, "flat" priorities would do (output or log) and exit). I agree it could be thought as convoluted, and, again, advice against combining flow control operators.

I believe this justifies the change proposed.

NB: I am preserving #no-and-or-or anchor (in addition to the new #and-or-flow anchor), in order to old links to this rule still work (but leading to the new rule).

[mikegee]

You make some strong arguments, but I'm still not convinced. Your best examples of their usage (under bullet 1. above), are not strictly superior to the 2-line versions of those expressions:

# value = fetch_necessary_value or raise Error, "Pretty long error message"

value = fetch_necessary_value
raise Error, "Pretty long error message" unless value
# or :)
value || raise Error, "Pretty long error message"

# value = fetch_optional_value or return

value = fetch_optional_value
return unless value
# or :)
value || return

# possible_error_check || other_possible_error && condition_for_that_error and raise

problem = possible_error_check || other_possible_error && condition_for_that_error
raise if problem
# or :)
problem && raise

While I'm not comfortable banning part of Ruby on principle either, I haven't seen code that was improved with and/or over many years of full-time Ruby development.

[bbatsov]

I'll not going to comment on this right away. I'd rather like to hear what @rubocop-hq/rubocop-core members think, @pirj, and everyone else who'd like to participate of course.

[zverok]

(I promise this is my last comment in the discussion to not turn it into flame, until very strong need to support my proposal with new, non-obvius arguments.)

@mikegee

Your best examples of their usage (under bullet 1. above), are not strictly superior to the 2-line versions of those expressions.

Well, first of all, I believe that for unbanning language feature a clear showcase of its non-confusing usage is enough, this usage should not necessarily be "strictly superior" (also, "superior" for whom?..).

Second, there are many styles and approaches, but I believe some general tendency towards DRYing up successive statements mentioning the same variable is an important part of lot of them.

So, especially in "precondition" context of the method (preparing and validating arguments), I believe that some superiority is clearly present:

# or version: we are fetching value this way, OR, if it isn't ther, we raise (something)
value = fetch_necessary_value or raise Error, "Pretty long error message"

# 2-line version: first, fetch the value. Then, next step, raise error (if something)
value = fetch_necessary_value
raise Error, "Pretty long error message" unless value
# Two problems here:
# 1. there are two statements, not one, so the relationship between them -- 
#     that the second is the mandatory fallback for first, is not immediately obvious
# 2. I haven't wrote "Pretty long error message" accidentally.

# Imagine this:
raise OurNamespace::SomeParticularError, "Expected value to be fetched when #{context}, but there were no. Try providing value." unless value
# vs this:
value = fetch_necessary_value or raise OurNamespace::SomeParticularError, "Expected value to be fetched when #{context}, but there were no. Try providing value."
# The second version obviously gives you the clear "fetch or raise" vision, plus
# an ability to format the message as a "tail":
value = fetch_necessary_value or raise OurNamespace::SomeParticularError, 
                                        "Expected value to be fetched when #{context},"\
                                        " but there were no. Try providing value."

# As about this:
value || raise Error, "Pretty long error message"
# I believe you are joking, because this is 
# a) just meaningless on a separate line and
# b) you'd better try it first, it can't be parsed by Ruby at all

I recall a dialog with a large group of very high-level colleagues on my previous work: and/or were banned by Rubocop there, but when we discussed this usage, at least half of them agreed it is more readable (and again, for just "unbanning" feature, not promoting or calling it superior to any other approach, I believe it is enough argument).

[pirj]

I'm bad at remembering things and prefer simple code that I wouldn't have to spend much time understanding, and preferably wouldn't have to look up operator order precedence in the docs.

The usage of or and and is quite uncommon, and I would prefer that they won't appear as the new thing to learn, and so I wouldn't have to remember it.

It's not correct that the guide advises using ||/&& instead of or/and in control statements:

For flow control, use if and unless

It's totally up to the project maintainers to turn off this or any other rule if they decide the developers working with the code on a regular basis are well aware of how exactly this works and should be used.

Would you agree with softening the wording from

The and and or keywords are banned.
to
Avoid the use of and and or keywords.
?

The use of one-liners is justified when the code is clean and readable. I'm afraid the example with a very long error message and a conditional doesn't fall into this category.
The below is my personal preference, I'm not putting it here as a reference example of how any of the snippets above should be rewritten, just an option to consider:

value = fetch_necessary_value
raise OurNamespace::SomeParticularError, <<~MESSAGE unless value
  Expected a value to be fetched when #{context},
  but there were none. Try providing a value.
MESSAGE

[Drenmi]

There are a lot of great rational arguments to be made for the use of these, and there are just as many rational arguments to be made against them. However, it doesn't matter how good it sounds in theory, if it doesn't work out in practice.

The empirical argument unequivocally supports the case against. When I started out with Ruby, I routinely encountered logic errors introduced by these. In later years, I haven't encountered any at all, because their use has largely disappeared.

On the other hand, so far I haven't come across a single place where they could make our code more readable or easier to understand.

In other words, my experience with these, after doing Ruby full-time for half a decade or so is exactly summarized in the Style Guide:

The minimal added readability is just not worth the high probability of introducing subtle bugs.

High risk, low reward.

For those reasons, I'm not too keen on recommending these to anyone. Whether the Style Guide should be updated from a "don't use" to a "use at your own risk" stance, I don't have a strong opinion.

[jonas054]

What if we limit the use of and and or to expressions where the right hand side is a "flow command" such as return, break, or exit? Would there still be a risk for misunderstanding of function and subtle bugs? Give me an example if you can come up with one.

This is something we could easily check in RuboCop, BTW.

[zverok]

OK, let me be bitter.

So far, all counter-arguments can be boiled down to exactly two statements:

1. Usage of and/or may produce "subtle bugs". No examples were provided here to support this claim, but I believe that it is about using of those operators in boolean context (ifs and calculation of boolean variables). Which can be properly rephrased into: "This (sane) language feature, when used in a wrong way, produces bugs". Well... Ain't all of them? IDK, shouldn't we ban bit &, because if somebody uses it instead of &&... BAD things will happen.
2. "I don't like it" (Don't see any value, don't think it is readable, or even, like the guide currently say, in a more cunny way "The minimal added readability doesn't worth the high probability..." -- argree that readability is "added", but make it look neglectible).

And (2) is exactly the problem I am trying to address. I believe that and/or operators fell into "negative feedback loop": borrowed from Perl, first, they were thought of by most Rubyists as "broken boolean ops"; then they were forgotten/advised against; then nobody has used them; then, if you try to tell someone "they are good for what they designed", people are like "ah, never seen them and I remember there were some bad things about". And nobody sees them because of it, and everybody just thinks "I never seen it" :)

In other words, people tend to constantly confuse "I haven't tried to use it" with "it is unreadable / not clean code".

The thing is (and I'll be bold here, the discussion seems to be in a dead-end anyways), var = something and/or raise/return idiom can be proven to be better easily:

• it is DRY (don't repeat the variable you've just calculated to check if it was good)—Rubyists are used to not repeating names, that's where Symbol#to_proc came from
• it is in a spirit of Ruby's "chaining statements related to the same value" (besides dot-chain you can also remember how popular even if questionable AS's .presence is)
• it is naturally readable and rememberable: I am saying it from experience with dozens of students I've exposed to this idiom, the typical reaction was intuitive understanding and joy
• it is handy to write/read (my example with raise and long error message)
• as any statement chaining, it can lead to better code (when trying to express meaning so that it would allow using this idiom, you will in many cases extract pre-checks/calculations of method arguments, indirectly making method's body readability higher).

But however long I'll make the list above, the answers would be "empirical": I don't see how it is readable, I don't like how it looks, I never saw it used in production, Python doesn't have it and it would be confusing, CoffeeScript uses and for logical operations and it would be confusing and so on. All saying mostly the same: I don't use it, so there should be something wrong with it.

Unfortunately, at the end of the day, I am maybe too old and disillusioned to fight those windmills. And Ruby's dying anyways :)

[bquorning]

The usage of or and and is quite uncommon, and I would prefer that they won't appear as the new thing to learn, and so I wouldn't have to remember it.

I wonder if not RuboCop is at least partially to blame for that.

Style/AndOr is one of the few cops that I always disable on a project. I write Ruby, I have read @avdi’s article, and I know how and and or works. I don’t like that the community style guide bans valid language features.

On the other hand, you don’t have to look very far to find popular projects using control flow keywords as boolean operators. The first three gems I checked gave three results:

• https://github.com/rack/rack/blob/5559676e7b5a3107d39552285ce8b714b672bde6/lib/rack/request.rb#L465
• https://github.com/carrierwaveuploader/carrierwave/blob/f32db18a3cd45a55d4e008990a814438a959ed6e/lib/carrierwave/uploader/url.rb#L18
• https://github.com/sinatra/sinatra/blob/b7c10649cad56f3263df089027621fb1753620cb/lib/sinatra/base.rb#L216

So while I may claim to always only use and and or for control flow, evidently they are often used as boolean operators.

[Drenmi]

This is something we could easily check in RuboCop.

I am willing to volunteer working on this, because I think it could serve as a good educational tool that might help bridge the gap to those using and and or prudently.

[matkoniecz]

as time went and approaches to Ruby style and coding practices became crystallized, it became obvious that and/or were designed for control flow

I would dispute it. Maybe this use became obvious for experts, but it is still not obvious for nonexperts.

[matkoniecz]

Also, I would not consider

# their different spelling and priority are good for mixing them with logical evaluation
possible_error_check || other_possible_error && condition_for_that_error and raise

as a readable code

[3limin4t0r]

@matkoniecz That is because you're doing too much in one same statement. You can make anything unreadable if you want to. At least it's clear that if # insert some logic evaluates truthy then an exception will be rasied. However, that might indeed not be the best example. But I find the variable assignment pretty readable.

# 1.
variable = something_that_might_return_falsey or return

# 2. 
return unless (variable = something_that_might_return_falsey)

# 3. 
variable = something_that_might_return_falsey
return unless variable

In my opinion option 2 is just plain bad. Whereas option 1 and 3 are both equally readable. For this reason I would go for option 1 since it's one line less. If you follow the default RuboCop guidelines a method may only have 10 lines of content. Changing two lines into one is pretty huge if you don't sacrifice any readability.

[matkoniecz]

For me:

2 is incredibly ugly and unclear.
1 is unclear
3 has one more line and is not using confusing constructs

I would save 1 style for code golf puzzles showing off tricky ruby semantics

[ka8725]

What about mix of logical and flow operator like the following?

this.is_not_right? and raise 'error' unless may_be_raise?

Is that good or bad? If bad, what's good alternative?

[mvz]

IMHO, it's bad because there are too many control flow keywords there, just like:

raise 'error' if this.is_not_right? unless may_be_raise?

A good alternative:

unless may_be_raise?
  this.is_not_right? and raise 'error'
end

(It seems the logic is inverted for the name may_be_raise?)

[zverok]

@bbatsov Is it possible to make, 4 years later, an executive decision and acknowledge that banning of language features was an early mistake of the style guide?.. The current situation is really honestly weird. The corresponding cop currently does the following:

• has conditionals as the default setting (directly contradicting the bold Style Guide's statement "The and and or keywords are banned. ")
• suggests that && is cool for control flow (which it is NOT)

# Cop's example:
# good
foo.save && return

# does it look good for you?..
render 'my_template' && return # parsed as render('my_template' && return)
# does it look good for you?
user.suspended? && return :denied # Syntax error

[bbatsov]

I'd phrase this more like a rune - e.g. Don't use and and or as logical operators...

[bbatsov]

@zverok Yeah, I will - let's allow them, but let's be more specific on the wording and add extra rationale (e.g. maybe a big sidebar and some note this has long been a controversial topic in the Ruby community).

[zverok]

@bbatsov Thanks. I rebased and reworded the PR on the new README.adoc, please take a look 🙏

[bbatsov]

Why fallback? I guess you meant final expression in a method in general, right?

[zverok]

@bbatsov I tried to adjust the wording, making it more explanatory. I also moved the "good usage" example to be the first one, so it also works as a demo of the concept.

[bbatsov]

whether -> Whether
use these -> prefer these operators over

[zverok]

🎉
I remember when we first met (on Kyiv RubyC in 2017? 2016) my very first phrases was: "Hi, nice to meet you! BTW, Rubocop is cool, but why are you so severe to and/or? They are cool too!"
Mission accomplished. 😁

Thanks.

[bbatsov]

Good things take a while... 😉

[WaKeMaTTa]

This is not good.

and and or should kept banned, because is confusing for beginners.