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

----- Original Message ----- > From: "Moti Asayag"<masayag(a)redhat.com> > 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...). >> i agree as well >> 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. > actually this is how it works today in most cases, i think its not bad, some checks relay on the fact that if this check is executed, then it's safe to do things, for example - first check validates vm is not null, second check use the vm, assuming its not null. i would go further and make the validation methods return boolean and call them in that way: public boolean canDoAction() { ...return checkTargetDomainHasSpace()&& checkTargetDomainIsValidTarget()&& checkSourceDomainIsValidSource()&& checkSourceAndTargetAreDifferent(); ... } private void checkTargetDomainHasSpace() { if(!targetDomainValidator.hasSpace(getParameters().getDiskImageId())) { addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW); reutrn false; } return true; } >> >> 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 > _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel