[Engine-devel] a different approach to the command classes
Ayal Baron
abaron at redhat.com
Tue Jan 17 20:14:34 UTC 2012
----- Original Message -----
>
>
> ----- Original Message -----
> > From: "Maor" <mlipchuk at redhat.com>
> > To: engine-devel at 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 at ovirt.org
> > >> http://lists.ovirt.org/mailman/listinfo/engine-devel
> > >>
> > > _______________________________________________
> > > Engine-devel mailing list
> > > Engine-devel at ovirt.org
> > > http://lists.ovirt.org/mailman/listinfo/engine-devel
> >
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> >
> _______________________________________________
> 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