Saggi Mizrahi has posted comments on this change.
Change subject: Refactor UpdateVdsGroupCommand
......................................................................
Patch Set 3:
I know that this is the philosophy but I claim that the other is confusing.
The idea is bail up first and bail out with a constant
When debugging you can easily know you don't have to care about the rest of the
methods as if one of them fails you bail out.
You KNOW the result you don't need to continue on reading the entire 10000 line
method to figure out what result is and will it be changed.
To change the flow to what you are describing, run all test but return false
even if one of them failed you change "return false" to result = false. The
important thing is not to check for falseness again.
I agree that when you have a random return hidden in a 1000 line functions at
the 10th indentation it might be confusing .
This is why the function is written in a way like.
if (there is nothing to check) {
return everything is OK
}
and not in how you say it
if (there is nothing to check) {
result might be false but continue checking on.
}
if (we didn't already fail && something) {
more code
}
To try and sum up. Everything is confusing when it's hidden in ugly code.
Bailing out early allows you to skip on needles indentation keep checking for
variable you don't need and confuse the reader.
What you say in early return is "If this fails, I don't care what happens next.
This is what the code original did but did not say. When you want to change
this logic. You will have to change the expressed logic again. But at least you
would think on when to break it. For instance when seeing the an arg is
actually null. There is no reason for me to ever try and work with it. If the
new value is equal to the current there is never a reason for me to check
anything as it was checked last time, return true.
--
To view, visit http://gerrit.ovirt.org/2271
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I47300fec2f6f5fabdbc75e8c78332c5aef2508a2
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Saggi Mizrahi <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
