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

----- 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 > _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel