Ayal Baron has posted comments on this change.
Change subject: Refactor UpdateVdsGroupCommand
......................................................................
Patch Set 3: (2 inline comments)
Omer,
"i really don't like the way some methods return true in the middle of it, as
it is hard to understand and harder to maintenance,"
Actually the "early returns" pattern is pretty widespread and an accepted
practice these days due to the fact that it simplifies the code and makes it
more legible.
The less nesting you have the easier it is to read it.
Normally when you run a series of checks you want to fail as fast as possible
and not go on performing more complex tests which might take time or be
redundant because previous tests fail etc.
But even more to the point is your own example:
" for example, if i would like to add a check in validateMemberVmsState method,
i should be really carefull where i put my code, as it will not always be
checked!"
The problematic behaviour you are describing is exactly how the code behaved
before the patch.
Each "if" statement that failed set "result" to "false" and the subsequent "if"
started with 'if result'
so the code wouldn't run those checks if previous checks failed. The semantics
are exactly the same now, it's just much easier to read though because the if
statements are shorter and focus on the matter at hand (no clutter by 'if
result' for example.
It's also easier to see that this is indeed the behaviour.
If you'll visually examine the patch without reading the code itself you will
note that it is easier to focus and hence review the code using the early
return pattern as it straightens the code and emphasizes the logic and removes
complex structures that do not deal with the function logic itself (e.g. "if
result &&")
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVdsGroupCommand.java
Line 155:
actionMessages.add(VdcBllMessages.VAR__ACTION__UPDATE.toString());
Actually this part wasn't changed from the original. However, in the code,
sometimes there are calls to getCanDoActionMessages and sometimes
addCanDoActionMessage.
Is there any reason for this? or is the preferred way to use the latter and get
rid of the former?
Line 193: if (!validateLocalfsSpecificRequirements()) {
It's easier on the eye this way. Otherwise you end up with a very long line
and you need to think harder to understand what it does.
--
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
