41

When indenting long if conditions, you usually do something like this (actually, PyDev indents like that):

if (collResv.repeatability is None or
    collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()

However, this puts the block started by the if statement on the same indentation level as the last part of the if condition which makes it very ugly/hard to read in my opinion as you don't immediately see where the block starts.

Some other styles I thought about:

if (collResv.repeatability is None or
        collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()

This looks pretty inconsistent as the second line is indented much more than the first line but it's readable.

if (collResv.repeatability is None or
  collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()

This is also more readable than the first example, but the indentation is not a multiple of 4 anymore and besides that it looks wrong as the second line has less indentation than the beginning of the condition in the first line.


So, my main question is: Is there a suggested indentation style for cases like that which do not require overly-long lines (i.e. a single-line condition)? If not, what do you prefer for cases like that?

martineau
  • 119,623
  • 25
  • 170
  • 301
ThiefMaster
  • 310,957
  • 84
  • 592
  • 636
  • Why isn't there a method on `collResv` to do the test? The fact that you have to do a compound condition on something that isn't `self` may indicate you need to refactor. Likewise why doesn't a method called `rejectCompletely()` also set the `rejected` attribute on the object? – Duncan Feb 25 '11 at 13:11
  • Actually that was just an example. there is no method with that name - i just wanted something sounding a bit sensible to have more than one line in the body. – ThiefMaster Feb 25 '11 at 14:02

9 Answers9

29

Often I work around this problem by calculating the condition in an own statement:

condition = (collResv.repeatability is None or
             collResv.somethingElse)
if condition:
    collResv.rejected = True
    collResv.rejectCompletely()

Though, for a still relatively short condition as in your specific example I'd go for nosklo's solution - the extra statement used here is more suited for even longer conditional expressions.

Oben Sonne
  • 9,893
  • 2
  • 40
  • 61
  • 1
    I think that code makes things less readable. There's really a variable just to avoid a multi-line if?! – David Ehrmann Nov 11 '13 at 19:51
  • 1
    The readability of this approach depends -- as often -- on your particular situation and of course is highly subjective. Besides that, I wouldn't care about the extra variable issue. After all that's what variables are there for: use short names for more complex things. Extra bonus: when debugging your code, some "redundant" variables come in handy. – Oben Sonne Nov 29 '13 at 21:17
  • The only time where this really makes sense is if your condition is reusable, otherwise this is mostly lazy. you could have a function that is something like `is_valid_thing` and then use that for your control flow. – TheBigFriezy Jun 30 '23 at 16:34
17

This is what I do:

if (collResv.repeatability is None or
        collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()
nosklo
  • 217,122
  • 57
  • 293
  • 297
13

This is an indirect answer--not answering the style question directly, but it's the practical answer in general, so it's worth mentioning.

I find it extremely rare to need to write multi-line conditionals. There are two factors to this:

  • Don't wrap code at 80 columns. PEP-8's advice on this subject is ancient and harmful; we're well past the days of 80x25 terminals and editors that can't sensibly handle wrapping. 100 columns is fine, and 120 is usually acceptable, too.
  • If conditions become so long that they still need to wrap, it's usually reasonable to move some of the logic out of the conditional and into a separate expression. This also tends to help readability.

Grepping through my recent projects, around 12kloc, there's only one conditional long enough that it needed to be wrapped; the issue simply very rarely arises. If you do need to do this, then as nosklo says, indent it separately--as you noticed, indenting it to the same level as the block beneath it is confusing and hard to read.

Glenn Maynard
  • 55,829
  • 10
  • 121
  • 131
  • 28
    Generally I agree, though there are still good reasons to stick to 80 columns (eyes prefer text with a limited width; possibility to work with multiple files side-by-side). – Oben Sonne Feb 25 '11 at 13:26
  • @Oben: That's a formatting principle for paragraphs, not code. When you wrap English paragraphs at a certain width, *every line* is around that width; this makes wide formatting hard to read because your eyes are constantly scanning the whole width. By contrast, when you wrap code at 120 columns, most lines are much shorter; there are just a few isolated lines which are longer. The readability issues associated with too-wide paragraphs don't occur. – Glenn Maynard Feb 25 '11 at 13:49
  • I don't find the "side-by-side" case interesting--two 80-column editors don't even fit on my 24" monitor at a comfortable font size, and in any case wrapping at 80 columns uglifies code so badly it's just not worth it. People can live with a bit of editor wrapping if they're in that configuration, rather than expecting everyone else to squish their code. – Glenn Maynard Feb 25 '11 at 13:59
  • Yes, a limited with is more important for continuous text. Still, I (respectively my eyes) prefer this principle for code too - but obviously this is a personal preference. – Oben Sonne Feb 25 '11 at 14:02
  • 9
    As soon as one dev uses superlong lines - thinking that 200-char lines or whatever don't have to wrap on today's screens - some other developer now can't put three windows side-by-side on a monitor and see reasonably formatted code. More than 80 cols is essentially the same as inflicting unwrapped code on other devs - it has nothing to do with prose text. Just because a given author can't put two 80 column editors side by side doesn't mean no one else will want to (although some devs needing a large font size to read easily is a good argument against long functions... AND long lines) – Alex North-Keys Apr 02 '14 at 22:00
  • The 80 lines constraint is explicitly stated in the google formatting guide for python: https://google.github.io/styleguide/pyguide.html#Indentation which is widely used across many different communities. – Tobbey Feb 06 '18 at 09:29
11

One problem with all previous suggestions here is that the logical operators for the subsequent conditions are put on the preceding line. Imo, that makes it less readable.

I recommend putting the logical operator on the same line as the condition it appends to the if statement.

This in my opinion, is better

if (None == foo
        and None == bar
        or None == foo_bar):

than this:

if (None == foo and
        None == bar or
        None == foo_bar):
Reimund
  • 2,346
  • 1
  • 21
  • 25
  • 1
    This is a comment, not an answer. – augurar Mar 21 '17 at 20:04
  • 4
    One of the questions asked is "...what do you prefer for cases like that?" My answer answers that question, so not just a comment. – Reimund Mar 22 '17 at 09:24
  • 1
    The OP is asking about indentation. Your "answer" is about where to put operators in multiline expressions. – augurar Mar 22 '17 at 18:13
  • 2
    It's actually not just a question about indents. The question is also about code style and legibility of code which the OP is clearly concerned about. – Reimund Mar 28 '17 at 07:53
  • 1
    This is a comment not an answer and the reverse is actually preferred, to end each line with "or", "end" or "else". – radtek Mar 29 '23 at 21:17
  • IMO, any answer whose primary reasoning is "in my opinion" is not a real answer. Unless you are a primary source qualified to make official/notable suggestions or you can cite someone/some organization who can, your opinion should be a comment. – Michael M. Mar 31 '23 at 20:57
3

I would do it this way. Keep it indented far away not to get confused.

if (collResv.repeatability is None or
                          collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()

PEP-8 advise is right here.

http://www.python.org/dev/peps/pep-0008/#indentation

Below code is advised

# Aligned with opening delimiter
foo = long_function_name(var_one, var_two,
                         var_three, var_four)

# More indentation included to distinguish this from the rest.
def long_function_name(
        var_one, var_two, var_three,
        var_four):
    print(var_one)

Below code is not advised

# Arguments on first line forbidden when not using vertical alignment
foo = long_function_name(var_one, var_two,
    var_three, var_four)

# Further indentation required as indentation is not distinguishable
def long_function_name(
    var_one, var_two, var_three,
    var_four):
    print(var_one)
ronak
  • 1,770
  • 3
  • 20
  • 34
3

PEP-8 actually seems contradictory here. While the example under "Maximum Line Length" shows the use of parentheses and a standard 4-character indent, the "Indentation" section says, with respect to function declarations, "further indentation should be used to clearly distinguish itself as a continuation line.". I don't see why this would be restricted only to "def" and not to "if".

mcote
  • 634
  • 6
  • 6
0

An option I sometimes use (although I'm not completely sold on its readability):

if (collResv.repeatability is None or
    collResv.somethingElse
):
    collResv.rejected = True
    collResv.rejectCompletely()

Possibly it would be more readable this way:

if (
collResv.repeatability is None or
collResv.somethingElse
):
    collResv.rejected = True
    collResv.rejectCompletely()
Blorgbeard
  • 101,031
  • 48
  • 228
  • 272
0

Pep-8 recommends the way you indented your original example.

Now if you're willing to fly in the face of the oh so sacred of style guides :-) you could move the operator to the next line:

if (collResv.repeatability is None
    or collResv.somethingElse):
    collResv.rejected = True
    collResv.rejectCompletely()

I'm not really a fan of this, I actually find your original syntax fairly easy to read and wouldn't spend much time monkeying with the indentation or line breaks.

stderr
  • 8,567
  • 1
  • 34
  • 50
0

In such a case, I would simply do:

if (collResv.repeatability is None or
    collResv.somethingElse):
    # do:
    collResv.rejected = True
    collResv.rejectCompletely()
eyquem
  • 26,771
  • 7
  • 38
  • 46