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

[Moti Asayag]

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

> 
> Livnat
> 
> 
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel