Re: [Engine-devel] a different approach to the command classes

From: "Moti Asayag" <masayag(a)redhat.com&gt; To: engine-devel(a)ovirt.org Sent: Tuesday, January 17, 2012 9:47:48 AM Subject: Re: [Engine-devel] a different approach to the command classes On 01/17/2012 09:05 AM, Livnat Peer wrote: > 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. > > 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...). >

> 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. > In addition, it prevents from independent validations (inside the same canDoAction) to report on several validation failures in a single execution instead of getting a single failure, rerun the command again, get another failure and on... This is the reason why the type of canDoActionMessages is a List.

> > Livnat > > > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel