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

----- Original Message ----- > > ----- Original Message ----- >> From: &quot;Maor&quot;&lt;mlipchuk(a)redhat.com&gt; >> To: engine-devel(a)ovirt.org >> Sent: Tuesday, January 17, 2012 4:33:19 AM >> Subject: Re: [Engine-devel] a different approach to the command >> classes >> >> On 01/17/2012 09:24 AM, Ayal Baron wrote: >>> >>> ----- 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. > > Yes I am. I just haven't written that code yet! > > >> I think, using shared canDoAction validation between commands might >> be a >> good idea also, it can be seen in operations such as add/update >> commands. >> In most cases, the update validation, is already based on the add >> command validation, sometime it is implemented with >> inheritance/external >> class, in other cases it is just implemented with multiple code. >>>> 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. >>>> > Our overuse of inheritance is one of the things I'm trying to > rectify. Inheritance wasn't added > to the language to facilitate reuse. It is there to represent object > hierarchies. In some cases > we abuse this to leverage code reuse. This is often a code smell > that the code is in the wrong > place to begin with. If both classes need the code and they are not > logically related in an object > hierarchy, the code really needs to be outside the classes. > > We have cases where the use of inheritance for reuse have gone too > far. For instance: > > public class RemoveVdsSpmIdCommand<T extends VdsActionParameters> > extends AddVdsSpmIdCommand<T> > > So this says that a RemoveVdsSmpIdCommand is a type of > AddVdsSpmIdCommand? That implies that I can > use a RemoveVdsSmpIdCommand anywhere that an AddVdsSpmIdCommand is > expected. Otherwise we have broken > one of the fundamental contracts of object oriented programming. > >>>> 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...). > From what I have seen those canDoActions are in the minority. The > overall goals are to increase > the readability and maintainability of the code so I would suggest > being pragmatic about any approach. > If doing it doesn't help achieve the goal, then don't do it. > > > One of the ideas I'm trying to convey here is that the command > classes should be fairly ignorant. > They should be responsible for knowing the list of steps involved in > a workflow, but not the details > of how those steps are carried out. Imo knowing both the steps and > their implementation violates the SRP. > > >>> 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. >>> > I need to get the multiple storage domains feature put away and then > I will > definitely do some refactoring to illustrate the value. I > wholeheartedly agree > that having actual code to kick around is better. I just can't do it > right now! > > >>>> 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. > Can you elaborate on your opposition to the use of guard clauses? > They have > been considered a best practice for quite some time. > >>> +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. > I'm not really a fan of using exceptions for controlling flow of > execution. Failing one of these > checks is not an exceptional event. Its an expected part of the > workflow and should > be handled as a normal occurrence. > > I would argue that wrapping the entire canDoAction method in a > try/catch would not make the code clearer: > > > public boolean canDoAction() { > boolean succeeded = true; > try { > do1(); > do2(); > do3(); > catch(ValidationException e) { > succeeded = false; > } > return succeeded; > } > > > has a lot more noise than: > > private boolean canDoAction = true; > > public boolean canDoAction() { > do1(); > do2(); > do3(); > return canDoAction; > } > You're forgetting the cost: instead of: public void do1() { ... } public void do2() { ... } public void do3() { ... } public void do1() { if (!globalSuccessVar) { return } ... } public void do2() { if (!globalSuccessVar) { return } ... } public void do3() { if (!globalSuccessVar) { return } ... }

I on purpose did not call it 'actionAllowed' to stress the fact that it's a global var (another thing I don't like).

And before you go to the one liner option per function, I consider 'if (!globalSuccessVar) return' bad form as it's not easy to discern what the condition is and what the action is.

> In the second form, there is no extra indentation or curly braces. > The validation steps > look just like a list of steps and imo it makes the code really easy > to understand by a maintainer. > >>>> >>>> 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 >> > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel >