bup icon indicating copy to clipboard operation
bup copied to clipboard

Removed redundant 'if'-statement.

Open br0ns opened this issue 12 years ago • 3 comments

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

I've removed the 'if'-statement.

br0ns avatar Apr 09 '12 15:04 br0ns

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.

lelutin avatar Apr 09 '12 21:04 lelutin

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.

br0ns avatar Apr 10 '12 07:04 br0ns

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.

lelutin avatar Apr 10 '12 12:04 lelutin