[Engine-devel] a different approach to the command classes
Roy Golan
rgolan at redhat.com
Tue Jan 17 13:25:49 UTC 2012
On Tue 17 Jan 2012 10:05:15 AM IST, Omer Frenkel wrote:
>
>
> ----- Original Message -----
>> From: "Moti Asayag"<masayag at redhat.com>
>> To: engine-devel at ovirt.org
>> Sent: Tuesday, January 17, 2012 9:47:48 AM
>> Subject: Re: [Engine-devel] a different approach to the command classes
>>
>> 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...).
>>>
>
> i agree as well
>
>>> 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.
>>
>
> actually this is how it works today in most cases, i think its not bad,
> some checks relay on the fact that if this check is executed,
> then it's safe to do things, for example - first check validates vm is not null,
> second check use the vm, assuming its not null.
>
>
> i would go further and make the validation methods return boolean and call them in that way:
>
> public boolean canDoAction() {
> ...return
> checkTargetDomainHasSpace()&&
> checkTargetDomainIsValidTarget()&&
> checkSourceDomainIsValidSource()&&
> checkSourceAndTargetAreDifferent();
> ...
> }
>
> private void checkTargetDomainHasSpace() {
> if(!targetDomainValidator.hasSpace(getParameters().getDiskImageId()))
> {
> addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW);
> reutrn false;
> }
> return true;
> }
>
>>>
>>> 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
>>
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
I agree that it works better mainly in long and complex checks (did
that in setupNetworks). Its better to encapsulate shared validation
code in validator classes instead of using inheritance for that.
It is also easier to test the validator than constructing a command to
test the canDoAction part.
I'll take advantage and a rule of thumb - to make the canDoAction clean
and simple make sure all parameter validation is done
via the validation framework.
code like if (getParameters().getSomeParam != null ) {
addCanDoMsg(msg) } shouldn't be a part of your canDoAction.
Instead use:
class SomeBaseParameters
@NotNull
SomeParam someParam
More information about the Engine-devel
mailing list