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

[Mike Kolesnik]

----- 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();

I suggest that we can use a POJO - ValidatorResult - to pass back the (obviously) result of validation.
This will contain a status and (default) message for the failure.

Then, can do action check can either interpret it themselves, or use a common method which will do it for them (by adding the message to the messages list, in case the validation had failed).
This will look something like this:
  boolean isValid = validate(SomeValidator.validateSomething(param1, param2, ...));

This will allow the validator class to be unaware of how the command actually handles the message, while at the same time keep the code clean & DRY.

> > >>>      ...
> > >>> }
> > >>>
> > >>> ...
> > >>>
> > >>>   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 agree that this is a problematic use of inheritance, and we should think of another way to model command classes internally that is not an abuse of OOP.

> 
> > >> 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.

+1 Exceptions are for unpreditable cases where you really son't have much choice what to do and should probably stop dead in your tacks, while validation is actually just asking "hey, is this thing ok or not?".

> 
> 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;
>   }
> 
> 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.
>