ruby-style-guide icon indicating copy to clipboard operation
ruby-style-guide copied to clipboard

"if not" vs "unless"

Open giddie opened this issue 10 years ago • 29 comments

Maybe a question more than an issue: I've gravitated toward using if not in certain circumstances, and I've figured out why:

In situations where I expect the condition to be false, and the code will normally run, I use unless:

unless name.empty?
  puts "Hello #{name}"
end

However, whenever I expect the condition to be true, and the code will not normally run, I use if not:

if not name == 'Paul'
  STDERR.puts "Unauthorized!"
end

Most people would probably use unless across the board, but in the latter example I'd find that difficult to understand at a glance, simply because the words "if" and "unless" have this semantic meaning associated with them. I'm interested in thoughts on this. This isn't something I particularly planned, so it's really not an attempt to over-complicate: I fell into this pattern of use because of my brain finding it easier to grok the keywords this way, and I eventually realised what I was doing. Thoughts?

giddie avatar Jul 08 '14 15:07 giddie

One of the things I've always loved about Ruby is its expressiveness; it's the closest I've ever seen to Knuth's purported dictum, "Programming is a literary act" without actually learning CWEB.

Read both versions of the second snippet (if not name == 'Paul' vs unless name ==Paul`) and imagine yourself discussing the code with a colleague or, better, a quasi-technical manager. Which sounds more like a description you'd use in conversation? That's usually the one I pick.

jdickey avatar Jul 08 '14 16:07 jdickey

That's an interesting observation; I suspect you're hinting that "unless" sounds more natural. Actually, I think there's a syntax issue here. Although "unless" sounds more correct in spoken English than "if not", consider the following:

Unless his name is Paul, raise the alarm.
If his name is not Paul, raise the alarm.

To me, the first sounds strange, because it implies that raising the alarm would be a normal thing to do (which pragmatically I know it is not). The second seems more natural because it implies an exception: in the unusual case where his name is not Paul, the alarm is exceptionally raised. Hope this makes some sense?

giddie avatar Jul 09 '14 07:07 giddie

OK, you've got a point there with the expanded, grammatically correct sentence. I'd still argue that the style guide (and automated checkers such as RuboCop) should accept either without an overriding preference; let the team maintaining that individual piece of code sort it out to their own satisfaction.

jdickey avatar Jul 09 '14 08:07 jdickey

Agreed: automatic checkers can't understand this kind of semantic distinction. However, I'd suggest that the style guide mention that it is acceptable to prefer "if not" over "unless" in certain situations. It would be nice to have something reasonably official to point to to defend my usage :p

FYI, you may also be interested in a similar semantic construct they use in the Linux kernel (mostly for compiler optimisation): http://kernelnewbies.org/FAQ/LikelyUnlikely

giddie avatar Jul 09 '14 08:07 giddie

If his name is not Paul, raise the alarm.

if name != 'Paul'
  raise 'the alarm'
end

CoffeeScript's is and is not operators really shine in situations like this, but I don't think operators are that bad.

fuadsaud avatar Jul 09 '14 16:07 fuadsaud

Also, it's worth taking a look at #325.

fuadsaud avatar Jul 09 '14 16:07 fuadsaud

@giddie I remember that bit from the kernel; it makes a lot of code easier to follow, as good language (or macro) constructs do. I'm perfectly happy with "the style guide [mentioning] that it is acceptable to prefer "if not" over "unless" in certain situations", especially with a couple of contrasting examples. I think that in fact is about the best we can do.

@fuadsaud Yes, #325 is directly relevant; I originally thought @giddie opened this issue because he hadn't seen that issue (and nobody, including me, has yet written up an actual PR for the language to address it that would have made this discussion largely superfluous). And no, that's not intended as a ding against anyone; this is probably a more concise summation of what we were talking about there.

jdickey avatar Jul 10 '14 03:07 jdickey

@fuadsaud Operators are also fine in my opinion. I rely on brackets, != and ! more in complex logic expressions (and avoid unless, not, etc...), whereas I use English keywords wherever the logic is simple enough that it doesn't require active thought to process :)

@jdickey Sorry, I had indeed missed #325.

giddie avatar Jul 10 '14 08:07 giddie

@fuadsaud Oh also, my example wasn't brilliantly chosen: the != operator is not always an option:

# GOOD
if not user.admin?
  raise "Unauthorized"
end

# BAD
unless user.admin?
  raise "Unauthorized"
end

The reason for this is that unless implies that the usual case will be to raise the exception, which is not the case. Raising the exception is unusual, so if not feels more natural to an English speaker. Of course, even better would probably be if user.is_not?(:admin).

giddie avatar Jul 10 '14 08:07 giddie

Yay, I'm glad somebody agrees with me! ;)

I agree that it would be nice to be able to use is and is not like in coffeescript (though in my last job unless was pretty much always used to replace if not in coffeescript as well). I know that this isn't about coffeescript, but one of the gotchas of coffeescript's natural language is that all of these compile to different things:

a is not b   # => a === !b
a isnt b     # => a !== b
not a is b   # => !a === b
not (a is b) # => !(a === b)

cheerfulstoic avatar Sep 28 '14 08:09 cheerfulstoic

Putting aside how the expression feels or sounds, if not is a bad idea because it is not logically or structurally equivalent to unless, and so you are setting up future developers to shoot themselves in the foot when they augment your code and get surprising syntax errors.

You start off with:

if not user.admin?
  raise "Unauthorized"
end

Then somebody comes along and adds an additional condition following your example:

if not user.admin? && not user.moderator?
  raise "Unauthorized"
end

BAM, cryptic syntax error:

why_not_is_bad.rb:1: syntax error, unexpected keyword_false, expecting '('
why_not_is_bad.rb:3: syntax error, unexpected keyword_end, expecting end-of-input

gkop avatar Nov 12 '14 18:11 gkop

Maybe, but can't the style guide specify this? Programmers are (or at least should be) capable of figuring these things out and the point of this repo is to establish style guidelines.

This reminds me of a similar example. At a previous job it was specified that within ActiveRecord where clauses that the symbol syntax should always be used and never the ? syntax. That is I would often see code like this:

where("id = :id", id: id)

Leaving aside the fact that you can use a straight up hash, it's much simpler and more readable to say:

where("id = ?", id)

Yes, if you add more conditions it gets harder to figure out which question mark goes with which argument. So perhaps a better rule is question marks for <= 2 arguments, symbols for > 2 arguments. The same is true here with if not, I think. For smaller conditions one way is better, for larger conditions another way is better.

Interesting, though, that the combination of && and not does that. I wonder if it has anything to do with the fact that or and and (and I'm thinking perhaps not) are not so much for boolean expressions but for flow control. (explained very will in this video from Avdi Grimm: http://devblog.avdi.org/2014/08/26/how-to-use-rubys-english-andor-operators-without-going-nuts/ )

cheerfulstoic avatar Nov 12 '14 21:11 cheerfulstoic

Agree on all counts with @cheerfulstoic; DWIM first, with effective, effortless communication of intent a very, very close second. ISTR we have a rule elsewhere in the Style Guide preferring and and or (and maybe not but probably not :grinning:) for flow control rather than logic?

jdickey avatar Nov 13 '14 08:11 jdickey

As an aside, the guide already covers not: don't use it.

I avoid it for the same reason I avoid and and or; they're not synonyms for !/&&/|| as is frequently assumed. They're different operators, with the lowest possible operator precedence and this leads to some surprising errors.

Fine:

a = !b

Error:

a = not b

@gkop posts a better example above of real-world problems being introduced by not.

I'm fine with the discussion, but we should move the topic of consideration from "unless vs if not" to "unless vs if !". Nobody should be using if not.

meagar avatar Nov 13 '14 13:11 meagar

I have also thought about this like @giddie @cheerfulstoic have

Unless there's a good argument against it, I would like this issue to be reflected in the guide.

I mean,

I would like this issue to be reflected in the guide, unless there's a good argument against it.

I think it works a little better in English than the equivalent code. e.g. unless condition then frobnicate doesn't read as well as frobnicate unless condition

Moar examples

# GOOD
unless user.admin?
  raise "Unauthorized"
end

# GOOD
if not user.admin?
  raise "Unauthorized"
end

# BAD
unless user.admin? || user.diplomat?
  raise "Unauthorized"
end

# BAD
unless user.admin? or user.diplomat?
  raise "Unauthorized"
end

# OK
if not user.admin? and not user.diplomat?
  raise "Unauthorized"
end

# INVALID
if not user.admin? && not user.diplomat?
  raise "Unauthorized"
end

# BETTER
if user.not_authorized?
  raise "Unauthorized"
end

in sum,

  • if unless looks weird, try to use a simple if
  • don't mix and/not/or with &&/!/||
  • only use and/not/or for logical comparisons

And see http://devblog.avdi.org/2014/08/26/how-to-use-rubys-english-andor-operators-without-going-nuts/

I really don't like it when guides say BAD when what you mean is AVOID unless you know what you're doing.

A note on the language:

  • "Avoid" means don't do it unless you have good reason.
  • "Don't" means there's never a good reason.
  • "Prefer" indicates a better option and its alternative to watch out for.
  • "Use" is a positive instruction.

I think that's a better way of thinking than GOOD/BAD/OK/OKISH. Whenever I prefer a style that the guide declares bad I feel kind of defeated when trying to explain to someone why it's actually not universally bad.

bf4 avatar Nov 14 '14 03:11 bf4

FYI, I would say this is bad, not good:

# BAD
unless user.admin?
  raise "Unauthorized"
end

...because it implies that raise is the expected condition, whereas an exception is pretty much by definition unexpected. I'd also suggest that you avoid negation in your BETTER:

# BETTER
if not user.authorized?
  raise "Unauthorized"
end

...because negated boolean methods get confusing really quickly when you start using them in more complex logic.

giddie avatar Nov 14 '14 09:11 giddie

@giddie This is absolutely fine to me:

unless user.admin?
  raise "Unauthorized"
end

unless does not imply anything to me about expectations, it's simply a more readable if !.

meagar avatar Nov 14 '14 14:11 meagar

@meagar That's why I created the issue: because I maintain that it does imply a semantic expectation. I've explained this in detail in the posts above.

(i.e. I'd appreciate an argument that sheds more light on the discussion than "it's more readable for me")

giddie avatar Nov 14 '14 14:11 giddie

Agreed "if not" != "unless" (at least not always, and maybe not even usually). I think you (and others) have just trained yourself to read it that way, but I doesn't always make sense as English.

On Nov 14, 2014, at 16:00, Paul Gideon Dann [email protected] wrote:

@meagar That's why I created the issue: because I maintain that it does imply a semantic expectation. I've explained this in detail in the posts above.

— Reply to this email directly or view it on GitHub.

cheerfulstoic avatar Nov 14 '14 17:11 cheerfulstoic

@giddie @meagar I think you're confounding the usage of unless with how to think about an exception.

I also think the condition length is part of the discussion. e.g.

# GOOD
fail "Unauthorized" unless authorized?

# BAD
food = user.favorite_food(all_the_foods) unless authorized?

# GOOD
unless authorized?
  food = user.favorite_food(all_the_foods)
end

Since, in the latter, it becomes easier to miss that this is a post condition.

bf4 avatar Nov 15 '14 05:11 bf4

@bf4 Condition length is certainly a factor. Beyond one or two conditionals, it becomes impossible to grok easily in plain English, and I need to switch into "logic mode". When that happens, I generally switch to using only if and C-type logic operators (&&, ||, !).

I don't think it makes any difference if we're talking about exceptions or some other action. In your final example, your use of unless implies that you expect authorized? usually to return false.

A couple of other examples I find interesting to consider:

unless not authorized?
  user.grant_unlimited_power!
end

The above would be appropriate in a section where the user is expected to be an admin. This block guards against the unexpected situation where a user should not be granted unlimited power (e.g. they're on some kind of probation). It's unusual to see a double-negative like this, but I actually think it's justified and more readable (due to English semantics) in the context of guarding against an unexpected condition like this.

if not permitted_to?(:perform, action)
  errors.add(action, 'is not permitted')
end

The above is another example of a block that is expected to run only in a minority of cases. It is expected that the action will in fact be permitted in the majority of cases, and the block will only run in the unusual case where it is not.

There are a few situations where there's some branching logic in a context where there is no clearly expected path. As the two paths get closer to having an equal expectation, I find that the semantic meanings of if and unless become less important. Often, the conditionals in that situation will be more complex anyway, and I'll fall back to thinking in logic (with if and C-type boolean operators) instead of relying on the semantics of English operators. I can't think of a great example for this at the moment.

giddie avatar Nov 17 '14 09:11 giddie

Would a good rule here be "Use if ! if the condition is expected to be false, and unless if the condition is likely to be true." ? Does that phrasing even make sense?

johana-star avatar May 02 '15 23:05 johana-star

The whole argument doesn't make sense as the given example is if not name == 'Paul' whereas the only reasonable way to write it is if name != 'Paul'.

In a referenced #325 I said:

do something if negative_expression > do something unless positive_expression raise if array.empty? > raise unless array.full?

"If you can use if without not [negation], prefer it to unless."

And this doesn't have anything to do with condition being rare or common.

...or your expectation for the expression to be true or false either.

Nowaker avatar May 03 '15 04:05 Nowaker

@strand I like your suggestion of phrasing. The only thing that I would say is that in my experience most of the time you'll expect either a true or a false result most of the time, but sometimes it's not clear how often it will go one way or the other. In those cases I think it's fine to prioritize making the condition positive and adjusting the if / unless accordingly

@Nowaker I'm not exactly sure what you're saying, but I agree that if name != 'Paul' > if not name == 'Paul'. I think if the method can be changed to express negation (be that #==/#!= or #valid?/#not_valid?) that that is preferable.

Actually, I think I understand what you're saying now, but I'll leave what I said ;) But yeah, I think rareness only comes into play when you only have a positive expression/method available to you.

cheerfulstoic avatar May 03 '15 08:05 cheerfulstoic

I like @strand's phrasing, but I think it's the opposite way round, isn't it?

# using if !
raise_alarm!  if !access_permitted?
do_something  if !expected_condition?

# using unless
open_door!  unless locked?
do_something  unless unexpected_condition?

So: if ! if the condition is likely, and unless if the condition is unlikely.

@Nowaker Also agree. In the OP, I suggested if not, but since then I have been shown that this is a bad use of the not keyword. My use of a comparison operator in my first example confused the issue somewhat. I avoided comparison operators in later examples for that reason.

giddie avatar May 05 '15 08:05 giddie

Sorry for a necropost and this may be an off-topic too, but this discussion just helped me to formalize the things in my head about another conditional construct "and/or".

If talking about Englishness and code understanding I tend to like

all_is_good? or return

some_variable = get_some_variable or raise

and not to particularly like

something_bad? and return

In other words I tend to replace the post-conditional unless with an or, but not so much an if with an and. So that reading only the beginnings of every method's line provides the happy path. But following this guidelines produces code like

!something_bad? or return

which looks weird

What do you think?

I understand that this may be not a proper place to discuss it, moreover this has almost definitely nothing to do with the automated check tools, cause they won't understand it. disregard_this_text if so (SWIDT)

dreyks avatar Mar 28 '16 13:03 dreyks

@drewda All of those are unnecessary. Avoid and/or to begin with and just use postfix if/unless

Use return if something_bad? and raise ... unless all_is_good? instead of something_bad? and return and all_is_good? or raise ...

If you're at all concerned about the "Englishness", the and/or versions are horrible.

meagar avatar Mar 28 '16 16:03 meagar

Also, to clarify, when having local operators is appropriate && and || are better. See:

https://github.com/bbatsov/ruby-style-guide#no-and-or-or

Though I think there are still some cases where and and or make sense. Avdi Grimm's screencast matches my thoughts:

http://devblog.avdi.org/2014/08/26/how-to-use-rubys-english-andor-operators-without-going-nuts/

cheerfulstoic avatar Mar 29 '16 01:03 cheerfulstoic

Yes, Avdi's blog is where it all started. He seems to use only the perl-style or though

dreyks avatar Mar 29 '16 07:03 dreyks