[Engine-devel] a different approach to the command classes
Moti Asayag
masayag at redhat.com
Tue Jan 17 07:47:48 UTC 2012
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
More information about the Engine-devel
mailing list