9:24 a.m.
----- Original Message -----
On 17/01/12 04:58, Jon Choate wrote:
> The way the command classes are written has bothered me for a
> while.
> While implementing the multiple storage domain features I am
> presented
> with the opportunity to create a new command from scratch. I gave
> some
> thought to what I would like the command classes to look like while
> balancing that the class must still fit in with the existing
> structure.
> So here is what I came up with. I'd appreciate any feedback.
>
> The Command class encompasses only the rules of what needs to be
> done.
> It relies upon Validator classes to determine if the canDoAction
> conditions have been met.
>
> @Override
> public boolean canDoAction() {
> ...
> checkTargetDomainHasSpace();
> checkTargetDomainIsValidTarget();
> checkSourceDomainIsValidSource();
> checkSourceAndTargetAreDifferent();
> ...
> }
>
> ...
>
> private void checkTargetDomainHasSpace() {
> if(!actionAllowed) return;
>
> if(!targetDomainValidator.hasSpace(getParameters().getDiskImageId()))
> {
>
> addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW);
> actionAllowed = false;
> }
> }
>
>
> Each of the checks follows a similar pattern of
> - guard clause to see of we already failed and bail if we did
> - test for failure of the condition
> - add failure message if needed
> - set variable to failed if needed
>
> Storing the success flag in a variable allows us to keep the
> canDoAction
> method readable as a series of commands and to allow it to be
> accessed
> by all the private methods without them having to pass it around.
>
> The execution of the command will follow a similar pattern where
> the
> command class will only know how to describe what needs to be done
> and
> to rely on supporting objects to handle the implementation of these
> steps. Getting the implementation out of the command classes will
> allow
> the commands to share validation and implementation details and
> remove a
> lot of the duplication that currently exists within the commands.
>
>
> How do people feel about this approach?
Hi Jon,
The scope of your proposal is changing the implementation of the
canDoAction method, I think that the title of this mail is a bit
misleading.
Actually I think Jon was suggesting to do the same in the command itself.
Basically what you are suggesting is to split the canDoAction
implementation into methods and then extract them from the command
class
to a shared class so they can be reused.
In many cases we can use (are using) inheritance for reusing code,
there
are cases where inheritance does not do the work and we can extract
to
external classes.
I think such a change is welcomed but on a needed basis, I think it
is
overkill for the general use case and will make the code more
cumbersome
(if the original canDoAction was 4-5 lines long...).
Jon, I think the burden of proof is on you here to show a real example and how it makes
the code clearer (e.g. change 2 commands which share similar checks).
Without 'seeing' it I don't think we would be able to appreciate the advantage
of your approach.
One thing I don't like in the above suggestion is the way you
validate
that the previous condition succeeded/failed. Having this condition
at
the beginning of each validation method is not a good approach IMO.
+1, if the previous check failed it should raise an exception, not rely on the next check
to bail.
It would be easier (less code, clearer) to wrap all the calls with a try catch clause (1
for all the calls), catch the specific exception that says the check failed and return
whatever you want to return.
Livnat
_______________________________________________
Engine-devel mailing list
Engine-devel(a)ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel