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

Removed redundant 'if'-statement. #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Removed redundant 'if'-statement. #11

wants to merge 1 commit into from

Conversation

br0ns
Copy link

@br0ns br0ns commented Apr 9, 2012

There's a 'while'-loop with this structure:
while cond:
foobar
if not cond:
break

I've removed the 'if'-statement.

@lelutin
Copy link
Contributor

lelutin commented Apr 9, 2012

hi there!

in order to get review on bup stuff from the whole community (apenwarr included), you should send this as a patch to the bup mailing list1. You don't need to join the mailing list to actually send your patch there, and the rule of thumb on the list is to always "reply-all", so that people outside the list can receive responses.

[1 1/2]: or you can send an e-mail directly to [email protected]

for a quick review here: I didn't read the surrounding code with too much attention, but your commit message doesn't actually justify the patch:

  • why is the if-statement actually redundant (e.g. is this condition already handled higher up? or can this actually never happen because of XYZ?)
  • why is it a good idea to remove it (e.g. adds maintenance to the code, or misleads developers, or whatnot)

I'd suggest you add those details to your commit message and send that out to the list. You'll get broader review there.

You can also join the #bup channel on irc.freenode.net for faster discussions. But patch review is usually conducted on the list.

@br0ns
Copy link
Author

br0ns commented Apr 10, 2012

Well, I was just trying to help. At the end of the while-loop there's an 'if'-statement checking the very same condition as the 'while'-loop. It then breaks when the condition is not met, i.e. the same as the 'while'-loop. Try and unroll the loop with pen and paper if this is not clear.

@lelutin
Copy link
Contributor

lelutin commented Apr 10, 2012

oh darn.. if I had looked at one line higher than the snippet was showing in the diff in github, I'd have seen it..

it is indeed useless. @apenwarr If you're ok with it, the patch is pretty simple and doesn't change any behaviour. You could simply apply this on your repository.

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

Successfully merging this pull request may close these issues.

2 participants