Opened 7 years ago
Closed 7 years ago
#28858 closed Cleanup/optimization (wontfix)
Remove 'else' after 'return' or 'raise'
Reported by: | Дилян Палаузов | Owned by: | nobody |
---|---|---|---|
Component: | Core (Other) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Tomer Chachamu | Triage Stage: | Someday/Maybe |
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)
Change History (9)
by , 7 years ago
Attachment: | no-else-after-return.patch added |
---|
comment:1 by , 7 years ago
Component: | Uncategorized → Core (Other) |
---|
comment:2 by , 7 years ago
Cc: | added |
---|
The generated CPython bytecode is usually the same, I see no benefit in applying this rule.
comment:3 by , 7 years ago
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 by , 7 years ago
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 by , 7 years ago
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 by , 7 years ago
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.
comment:7 by , 7 years ago
Triage Stage: | Unreviewed → Someday/Maybe |
---|
The reporter already provided a patch and I created a PR. Opinions welcome.
comment:8 by , 7 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
The patch has gone stale and the discussion on the PR didn't yield a strong consensus. I think the value of trying to enforce some guidelines around this style is minimal.
Personally, I think the current code with elif has some readability benefits, at least in some places.