Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #28858 -- Removed unnecessary "else" statements. #9498

Closed
wants to merge 1 commit into from

Conversation

timgraham
Copy link
Member

Opinions on this patch are welcome. I'm not sure if it's an improvement.
https://code.djangoproject.com/ticket/28858

@srinivasreddy
Copy link
Contributor

srinivasreddy commented Dec 29, 2017

+1 I like this programming style and follow always. I am also interested in hearing out opinions.

@orf
Copy link
Contributor

orf commented Jan 2, 2018

I also like this, it seems cleaner and reduces indentation which is a plus. Is there a flake8 plugin we can enable on the linter for this?

@ngnpope
Copy link
Member

ngnpope commented Jan 3, 2018

+¾ - I largely agree with what Adam said in the ticket on this - where it doesn't impact readability, which is fine in the majority of cases.

Is there a flake8 plugin we can enable on the linter for this?

I couldn't find one with these searches, but I only had a quick look:

I also think that removing redundant elif leads to more readable code:

 if condition0:
     return 0
-elif condition1:
+if condition1:
     return 1
-else:
-    return 2
+return 2

If only Python had PERL's return value if condition syntax...

@jonashaag
Copy link
Contributor

I think this is a disimprovement whenelif is used to distinguish between disjoint cases, where you'd use switch ... case or cond (Lisp) in other languages.

Reason 1: code structure

switch ... case is the most readable construct for case distinctions:

# Very generalised form of switch ... case
switch:
  case a(foo):
    ...
  case b(foo):
    ...
  case c(foo):
    ...
  otherwise:
    ...

The closest you can get in Python is

if a(foo):
  ...
elif b(foo):
  ...
elif c(foo):
  ...
else:
  ...

Compare this to:

if a(foo):
  ...
if b(foo):
  ...
if c(foo):
  ...
...

It dedents the else case, losing the visual clue that this is one of the 4 cases that are being considered.

Reason 2: Additional reasoning complexity

switch ... case and elif cascades make it very clear that these cases are disjoint (that is, ignoring implicit fallthrough, which is horrible design). Multiple if blocks without elif require you to parse every single if block for return statements (etc.) in order to understand the code structure. So by having less visual and semantic structure, multiple if blocks have higher reading/understanding complexity.

Reason 3: No advantages?

Not sure here, but I don't see any advantages of using if over elif cascades other than that you can lose one level of indentation in the "otherwise" case, and the otherwise case only, which isn't really that much of an improvement considering that all other cases are indented anyways.

@timgraham
Copy link
Member Author

Yes, that was along the lines of my thoughts. Not using elif doesn't make it obvious that the conditions are mutually exclusive - you have to check each branch for a return statement. See https://stackoverflow.com/a/22783232 for an example.

@srinivasreddy
Copy link
Contributor

But it makes sense to raise exceptions with stacked if conditions.

if something:
   raise SomeExeption(....)
if another:
   raise  AnotherException(...)

I find the above pattern much better than

if something:
   raise SomeExeption(....)
elif another:
   raise  AnotherException(...)

@jdufresne
Copy link
Member

Just my personal opinion, I prefer the style proposed in this PR for the following cases:

  1. Raising an exception inside an if statement. It is understood as an exceptional case that immediately returns early.
  2. Return early guard clauses for the same reason as exceptions.

I do not prefer this style for:

  1. chained if/elif that look like switch/case statements of other languages. My preference here is the same as others have said, it makes the mutual exclusiveness less clear.
  2. if/else return statements on the same line without additional logic. For example, I don't think the following should have the else removed:
if some_condition():
    return 42
else:
    return None

@timgraham
Copy link
Member Author

Is the point of #2 to have the return statements lined up?

@jdufresne
Copy link
Member

Right. Although, I'd probably write that example as return 42 if some_condition() else None. Either way, I think it is only my personal preference and nothing more. I don't feel it must be codified in the style guide unless there is a lot of agreement or provides a measurable readability or performance improvement (which I don't believe is the case).

@timgraham
Copy link
Member Author

I'm not going to spend more time on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants