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

[Maor]

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.

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