[Engine-devel] a different approach to the command classes
Ayal Baron
abaron at redhat.com
Tue Jan 17 07:24:41 UTC 2012
----- 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
More information about the Engine-devel
mailing list