24

What is considered better practice?

Case 1:

if n == 0:
    doThis()
elif n < 0:
    doThat()
elif n > 0:
    doSomethingElse()

Case 2:

if n == 0:
    doThis()
elif n < 0:
    doThat()
else:
    doSomethingElse()

The first case is more explicit and (arguably) clearer.

The second case doesn't handle the incorrect case when n is not an integer. Pass in a non-integer object (one that implements __lt__ and __eq__) and it returns without throwing error. This isn't expected behavior. If this was written in a statically-typed language, this wouldn't be a problem. But this is Python.

The second case is, however, potentially quicker, because it avoids an extra conditional expression evaluation (i.e., the n > 0 statement).

9
  • 12
    Not a full answer, but the main difference between the two styles are whether you want a default option, or exclusively specific options. So the answer is likely "it depends".
    – noughtnaut
    Oct 9, 2023 at 11:36
  • 2
  • 1
    @NotThatGuy Not necessarily a code smell. Design-wise, if n's operations are vectorized, n == 0, n < 0, n > 0 may all return empty collections, which might be reasonably considered falsy.
    – InSync
    Oct 9, 2023 at 14:26
  • 7
    The first case also returns without an error if none of the conditions are true. It just never calls any of the doXXX. The second version lumps "none true" in with "n > 0". Either could be wrong.
    – Barmar
    Oct 9, 2023 at 14:53
  • 4
    @NotThatGuy Do you consider floating point variables "code smell"? Because a floating point value of NaN would behave exactly like that. (any comparison with a NaN value returns false. Including NaN == NaN).
    – Philipp
    Oct 11, 2023 at 9:11

6 Answers 6

24

Short answer

I would argue that there is no singular correct answer to this question.

It depends on what you want to have happen when one of the above cases changes. Are you assuming that the last case catches everything that's left? While the previous question may seem an obvious rephrasing from asking if you want it to work the way an else case works, sometimes the simplest questions are the ones that get you to the rightest answers.

Much longer answer

When you ask what is better, "better" here includes checking which is best logically, readably, speedily, and maintanably.

Logically, they're equivalent.
Edit: I was not aware of NaN in Python being an option here. The two options are only logically equivalent if they both cover every possible value, but NaN is not being covered for here. However, if you could somehow guarantee that NaN would not be a possible value here (e.g. this already being established in an earlier part of the code), then they would be logically equivalent.

Readably, the elif option is the most informative as it describes the case. However, I could make the point that a code comment would achieve the same purpose. So while the elif option slightly favors readability, if we find a reason to favor the else option for other reasons, we can mitigate this by adding a comment to the else option.

Speedily, the else variant is better due to having one less redundant consideration. That being said, what I just said only applies if this last condition fully covers every possible scenario that is left (i.e. after all the other conditions have turned up false already), and if you care about this level of performance. Generally speaking there is not such a performance constraint, and the question being asked here is more one of readability and maintenance (as so many good practice questions are).

So far, elif favors readability (but only by a tiny margin) and else favors performance (but only in relatively rare scenarios). I'm going to call this mostly a tie.

Maintainably, this is where the context matters. You used an example of a complete set (all integer numbers), and you picked cases that together form the whole set. There is no reasonable expectation that new numbers will be invented. Therefore, the last condition of the elif option is truly redundant.

However, you've unknowingly cherrypicked an example that introduces a constraint that doesn't always apply. I suspect you were asking a much broader question, so I'm going to use a different example that addresses a much broader set of use cases.

Let's say we have a company with five employees. Two of the staff are managers. The other three are developers. In this company, managers earn $150k, developers earn $100k, and no one has (or will ever have) personally negotiated salaries.

You can now rephrase your question to this example, which asks whether this ...

if(employee.Role == "Manager")
    salary = 150000;
else
    salary = 100000;

... or this ...

if(employee.Role == "Manager")
    salary = 150000;
else if(employee.Role == "Developer")
    salary = 100000;

... is better. You may have already noticed what's different about this example compared to yours: this is not a complete dataset. Over time, it's possible that you might hire an accountant. What should their salary be?

I very intentionally kept the salary rules vague, leaving it unspoken how the rule is designed. There are two main possibilities here:

  1. Everyone earns the same salary, but managers get a $50k bonus.
  2. Each role has a specifically agreed upon salary.

In the first case, an accountant inherently earns the same as a developer. Therefore, the else case is the better option, because it accurately separates the managers from "everyone else" (notice the "else" there).

In the second case an accountant's salary and a developer's salary have no relation to each other, and therefore the logic should not group them together. Therefore, the elif case is the better options, because it ensures that the salary amounts are correctly tied to the role that they belong to.

The question here is what happens when you forget to update this logic. With the else option, accountants will immediately earn the same as developers, which matches the first case listed here. With the elif option, accountants will not be given a salary until someone has explicitly decided what it should be, which matches the second case listed here.


Conclusion

Because I can't resist the opportunity here:

if(performance_matters_a_lot)
    return "else";
else if(current_logic_represents_final_and_complete_set)
    return "else";
else
    return "elif";

Yes, I did doublecheck that using else was consistent with the information in this answer.

8
  • 2
    I love the points addressed here. But this misses the (IMO) biggest point by declaring "Logically, they're equivalent". You can't correctly select an if-style without recognizing the logical impact they have when your assumptions about values are (inevitably) incorrect. Oscar's answer illustrates this quite well. Combined, your answers cover it all wonderfully Oct 10, 2023 at 18:51
  • @Indigenuity: I'm no Python expert and wasn't aware there was a non-numerical value possible here. If this were a C# integer, OP's cases would completely cover the possible values and then the two presented options would be logically equivalent. But you're right about this because it's Python, so I'm adding an asterisk to this part of my answer.
    – Flater
    Oct 10, 2023 at 23:05
  • 7
    I'd put an else: throw Exception('unhandled value') if doing an elif chain. It might seem redundant but, using your salary example, the code you provided will be logically flawed and potentially difficult to notice when it doesn't explode when accountants are added to the system.
    – Shadow
    Oct 11, 2023 at 2:25
  • @Shadow: How the code should behave when a new option is introduced is subjective/contextual and there's no one correct answer to cover all use cases. Exceptions are not necessarily the correct behavior and I intentionally stay away from that kind of blanket advice in this answer.
    – Flater
    Oct 11, 2023 at 2:31
  • 2
    @Flater Doing nothing maybe not correct behaviour either, so an exception is the better option because the error then doesn't slip by silently. Even if doing nothing would be the correct behaviour this should be said explicitly in the code.
    – BlackJack
    Oct 11, 2023 at 14:16
57

Even Case 1 can have unexpected behaviour. The value of n that worries me is NaN.

n = float('nan')
if n == 0:
    doThis()  # Not triggered
elif n < 0:
    doThat()  # Not triggered
elif n > 0:
    doSomethingElse()  # Not triggered

This would be a problem if the rest of the program assumes that one of these has been run. To avoid this you could raise an exception from a final else:

if n == 0:
    doThis()
elif n < 0:
    doThat()
elif n > 0:
    doSomethingElse()
else:
    raise Exception
10
  • 11
    This. Either use an else for a catch-all, or use else for the last case with an assert. In either case, double-check that you did account for all situations. Oct 9, 2023 at 13:03
  • 26
    For the fast reader (skimming over longer code) using else will make clear "one of these cases will be executed for sure". Using only if/else-if always implies "this may be optional - there may be cases which skip over this block". (and the compiler may see it the same way, e.g. warning about an uninitialized variable, if you have no else case)
    – Falco
    Oct 9, 2023 at 13:22
  • 3
    I think one thing to note is that although small things like this might feel more right to one person or another and are perfectly fine to discuss when writing your own code - don't let it impede team PRs because it's too small to start a debate over. I've gotten more and more frustrated over the years at different employers where these conversations take up way too much time when instead our team could be moving forward.
    – aaaaaa
    Oct 9, 2023 at 19:23
  • 7
    @aaaaaa: I'm not a fan of letting bugs in in the name of "friendliness" or "productivity". Instead, I'd recommend having the discussion once, with the team, coming to an agreement, and making it a review guideline with a link to the summary of the discussion. Anybody who wishes to challenge the discussion in the future is welcome to, but should first acquaint themselves with the summary and ensure their arguments are genuinely new. Oct 10, 2023 at 10:05
  • 3
    Doing relational logic on floats is not for the feint of heart.
    – T.E.D.
    Oct 10, 2023 at 13:36
2

In the specific case of n > 0 vs n < 0 vs n == 0, I think it's best to write the == case last and use else for it, but with a comment, like this:

if n > 0:
    ...
elif n < 0:
    ...
else: # n == 0
    ...

It is easier for a human reader to see at a glance that the comment is true if the cases are listed in this order, than if n == 0 is one of the first two cases.

Your concerns about supplying an argument of the wrong type to this function would be better addressed with type annotations.

3
  • 4
    I try to avoid arguments that explain "what". In this case, if I change the first condition to n > 1 and forget to edit the comment (it may be further down if the body of the if is longer) then the comment will be wrong. - I try to use comments only when I explain why not what
    – Falco
    Oct 9, 2023 at 13:18
  • I understand where you're coming from, but in this case you must edit two places anyway, so a third place is not much worse. And I think of this particular comment as a "why" comment, because it explains why you don't need elif here.
    – zwol
    Oct 9, 2023 at 14:02
  • 1
    I won't need to edit in two places, if the else-behavior suits my needs. Example: before n>0 was 'good' n<0 was 'bad' and everything in between is 'average'. If I increase the limit for 'good' to n>1 I don't need to edit anything else, but the comment is now out of date and will confuse the reader.
    – Falco
    Oct 10, 2023 at 9:09
2

Your own primary objection to your version 2 (with the unconditional else) seems to be that it acts wrongly when the type of the variable is not integer.

I'd argue that your first version is wrong as well. If n is not an integer, it will not enter any of the three cases and do nothing. This would not be the expected behaviour - I would want to have some indication that something went very wrong. Just silently swallowing the error condition by doing nothing is the same (in my opinion) than erroneously doing something. Both can be incredibly frustrating to debug, and can cause untold harm and suffering to your data.

So. If your data can be wrong in some ways (be it type, domain, etc.) then you should do error checking before using the data. This is obviously mandatory when using data that comes from the outside.

For a method like this, modern Python does offer type checking of arguments (arguably only in IDEs etc, not enforced).

To answer your original question; my personal preference is the style with a dangling unconditional else. And if I'm in doubt, then that case will be pure error handling (i.e., raising an exception if the language supports it).

I.e.:

if (someChecksFailed(n)):
  raise ValueError(f("Invalid argument: {n}")

if a(n):
  doA()
elif b(n):
  doB()
else:
  raise ValueError(f"Huh? Unexpected argument: {n}")

Or, if I'm very sure that nothing can really go wrong after my checks:

if (someChecksFailed(n)):
  raise ValueError(f("Invalid argument: {n}")

if a(n):
  doA()
elif b(n):
  doB()
else:
   doC()      

Finally, if "doing nothing" is a perfectly valid possibility, you can make it explicit:

if (someChecksFailed(n)):
  raise ValueError(f("Invalid argument: {n}")

if a(n):
  doA()
elif b(n):
  doB()
else:
   None      

Performance differences should be so irrelevant here that I would never take them into consideration at this point - if and when it turns out that that spot is for some reason performance sensitive then it's time to optimize, but this kind of construct should first and foremost be correct and easy to maintain.

5
  • "modern Python does offer type checking of arguments" - note that the typing module just provides type hints. It's up to your IDE or third-party tools to actually do type checking. And no runtime type checking is done.
    – NotThatGuy
    Oct 9, 2023 at 13:36
  • Thanks @NotThatGuy, I have modified my statement.
    – AnoE
    Oct 9, 2023 at 14:08
  • 5
    If you want to explicitly do nothing, pass is better than None, since the latter looks like you're using the value for something.
    – kaya3
    Oct 9, 2023 at 23:25
  • "If n is not an integer, it will not enter any of the three cases and do nothing." Hence the reason I added this: "Pass in a non-integer object (one that implements __lt__ and __eq__)". Oct 10, 2023 at 4:37
  • @AnoE There are many types besides int that could end up in a branch, so ”type checking” might be unnecessary restricting. float, decimal.Decimal, fractions.Fraction, just to mention a few from the standard library. And countless types defined in third party modules. On the other hand if you pass in a str value, this won't just do nothing silently but fail with a TypeError at the < comparison.
    – BlackJack
    Oct 11, 2023 at 14:30
2

The real problem is: You are sure there are the three cases n>0, n=0 and n<0, but people who are sure they are right are often wrong. And you would want to express that.

Swift has a switch statement where the compiler requires a guarantee that every case is handled. In your case, you write your three cases. The compiler checks that everything is covered. If not then you must add another case handling the missing values, or a “default” statement. If everything is covered, “default” is not allowed. If the compiler cannot check it, it’s not allowed.

So you would write down your three cases, and everything is clear. Two cases plus default means there might be cases that you missed. Three cases + default is not allowed.

(In another post: What if I change “n > 0” to “n > 1”? The compiler would figure out that not everything is covered and force you to fix it somehow. If you wrote two cases + default then the default would now unexpectedly include the “n = 1” case.

An easy fix in any language would be instead of a final else or else if have a statement “remaining n > 0” which implies an assert that everything is covered, with the compiler optimising things.

1

As you mentioned python is dynamically typed, hence either ensure using numbers or use elif n > 0:.

(Could even apply some comparison three valued type. Like signum. Or an else exception.)

But you favor speed. The overhead may exists, may be optimized away (in python?), but is minor.

Better optimize by probable number frequency (most frequent first):

if n > 0:
    ...
elif n < 0:
    ...
elif n == 0:
    ...

For a larger number of cases, nest if statements.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.