Opened 2 weeks ago

Last modified 11 days ago

#28858 new Cleanup/optimization

Remove 'else' after 'return' or 'raise'

Reported by: Дилян Палаузов Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Tomer Chachamu Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

... as it has no added value.

Attachments (1)

no-else-after-return.patch (147.4 KB) - added by Дилян Палаузов 2 weeks ago.

Download all attachments as: .zip

Change History (7)

Changed 2 weeks ago by Дилян Палаузов

Attachment: no-else-after-return.patch added

comment:1 Changed 2 weeks ago by Tim Graham

Component: UncategorizedCore (Other)

Personally, I think the current code with elif has some readability benefits, at least in some places.

Last edited 2 weeks ago by Tim Graham (previous) (diff)

comment:2 Changed 13 days ago by Tomer Chachamu

Cc: Tomer Chachamu added

The generated CPython bytecode is usually the same, I see no benefit in applying this rule.

comment:3 Changed 13 days ago by Дилян Палаузов

Skipping else after return, when it does not impact readability, leads to less code, and the resulting code is faster to read (by humans).

Currently it is unclear, when useless else shall be written.

comment:4 Changed 13 days ago by Tomer Chachamu

I think you answered your own question there - it is used when it impacts readability (as judged by the original authors, who may not always agree with each other).

An if..elif..else.. block clearly lists mutually exclusive branches taken, and can be seen by looking at a single indentation level. if..return; if.. return;.. requires checking two indentation levels to ensure there's a return/raise ending every if.

For example, in the first hunk of the patch, the code is:

if bla:
   some code
if bla2:
   raise
elif bla3:
   raise
return val

After the change, it is:

if bla:
   some code
if bla2:
   raise
if bla3:
   raise
return val

A newline before if bla2 would improve readability, however the elif is already helping to show there is some similarity between if bla2 and if bla3, and that if bla has a different purpose.

comment:5 Changed 13 days ago by Дилян Палаузов

python has no switch..case, but "elif", which is not the same. The readability criterion is very subjective one, e.g. the author might not have been aware that the 'else' was useless. As for

if x:
    return y
else:
    raise z

the else does not add for readability.

The first snippets of django/contrib/gis/gdal/srs.py or django/contrib/gis/geoip2/base.py I sent, have nothing to do with readability.

comment:6 Changed 11 days ago by Adam (Chainz) Johnson

So I'm very slightly in favour of removing the useless 'elses', I also consider it a tiny bit more readable, and amazingly cpython *does* generate less code in this case (it's no faster though, it's just removing an extra two opcodes from a redundant return None):

In [6]: def foo():
   ...:     if foo:
   ...:         return 1
   ...:     else:
   ...:         return 2
   ...:

In [7]: def bar():
   ...:     if bar:
   ...:         return 1
   ...:     return 2
   ...:

In [8]: dis.dis(foo)
  2           0 LOAD_GLOBAL              0 (foo)
              2 POP_JUMP_IF_FALSE        8

  3           4 LOAD_CONST               1 (1)
              6 RETURN_VALUE

  5     >>    8 LOAD_CONST               2 (2)
             10 RETURN_VALUE
             12 LOAD_CONST               0 (None)
             14 RETURN_VALUE

In [9]: dis.dis(bar)
  2           0 LOAD_GLOBAL              0 (bar)
              2 POP_JUMP_IF_FALSE        8

  3           4 LOAD_CONST               1 (1)
              6 RETURN_VALUE

  4     >>    8 LOAD_CONST               2 (2)
             10 RETURN_VALUE

However I doubt it's worth anyone's time to go through Django removing them. Perhaps we can add it to the style guide now, close this ticket, and fix it as code gets modified in the future.

Note: See TracTickets for help on using tickets.
Back to Top