Skip to content

Refactor unnecessary else / elif when if block has a return statement #4606

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

Closed
wants to merge 1 commit into from
Closed

Refactor unnecessary else / elif when if block has a return statement #4606

wants to merge 1 commit into from

Conversation

Borda
Copy link
Contributor

@Borda Borda commented Oct 12, 2021

Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>

…tatement

Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: Jirka Borovec <[email protected]>
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Borda

Personally, I don't think these changes are really necessary. I think most of them are a matter of taste, and to be honest I tend to think that some of them actually hurt readability of the code.

For example changing elif into if now forces the reader to read what's inside the first if block to understand that all blocks are exclusive with one another. It's usually more obvious with the original elif.

The same goes for

if something:
    return 1
return 2

I find that what we have right now is faster and easier to parse, especially when the first return is deeply nested within the first block.

if something:
    return 1
else:
    return 2

The only case where removing the else clause helps is when the else clause is much longer than the first one, i.e. the first if is just returning early.

Considering that the PR doesn't fix bug and is mostly a matter of taste, I would prefer not to merge this to not add more noise to git blame

factor = int((output_max + 1) // (input_max + 1))
image = image.to(dtype)
return image * factor
input_max = _max_value(image.dtype)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is typically a case where the change is a net negative in terms of understanding what's hapenning IMHO

@Borda
Copy link
Contributor Author

Borda commented Oct 13, 2021

Personally, I don't think these changes are really necessary. I think most of them are a matter of taste, and to be honest I tend to think that some of them actually hurt readability of the code.

I agree that this can be very opinionated, I was mainly reacting on #4585 (comment)

@datumbox
Copy link
Contributor

Same here. OK I'll close the PR then, thanks for the contribution nonetheless!

@datumbox datumbox closed this Oct 13, 2021
@Borda
Copy link
Contributor Author

Borda commented Oct 13, 2021

How about the comment/requested changes from @prabhat00155?

@NicolasHug
Copy link
Member

If you're referring to the changes you made in #4585 (i.e. d906910) , it's just a few lines, so while they're unrelated with the original goal of the PR, it's probably not worth debating over

@prabhat00155
Copy link
Contributor

Well elif after return doesn't make sense to me and depending on what linter you use, it may throw a warning as well(example pylint). Anyways, as most people are in favour of leaving it as it is, I am not going to push for the change. Thanks!

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

Successfully merging this pull request may close these issues.

5 participants