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

17 Jan 2012

      ----- Original Message -----
...
----- Original Message -----
...
From: "Maor" <mlipchuk@redhat.com>
To: engine-devel@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();
     ...
}
...
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 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.
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;
  }
You're forgetting the cost:

instead of:
public void do1() {
    ...
}

public void do2() {
    ...
}

public void do3() {
    ...
}

public void do1() {
    if (!globalSuccessVar) {
       return
    }
    ...
}

public void do2() {
    if (!globalSuccessVar) {
       return
    }
    ...
}

public void do3() {
    if (!globalSuccessVar) {
       return
    }
    ...
}

I on purpose did not call it 'actionAllowed' to stress the fact that it's a global var (another thing I don't like).

And before you go to the one liner option per function, I consider 'if (!globalSuccessVar) return' bad form as it's not easy to discern what the condition is and what the action is.
...
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.
...
...
...
Livnat
_______________________________________________
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel
_______________________________________________
Engine-devel mailing list
Engine-devel@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-devel