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

[Omer Frenkel]

----- Original Message -----
> From: "Moti Asayag" <masayag at redhat.com>
> To: engine-devel at 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 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
>