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

[Ayal Baron]

----- 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 at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>