ruby-style-guide
ruby-style-guide copied to clipboard
"if not" vs "unless"
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?
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.
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?
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.
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
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.
Also, it's worth taking a look at #325.
@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.
@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.
@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)
.
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)
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
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/ )
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?
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
.
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 simpleif
- 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
.
- "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.
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 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 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")
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.
@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 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.
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?
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.
@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.
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.
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)
@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.
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/
Yes, Avdi's blog is where it all started. He seems to use only the perl-style or
though