[Engine-devel] a different approach to the command classes
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?
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. Livnat
----- 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.
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@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
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@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
----- 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; } 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
----- 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();
I suggest that we can use a POJO - ValidatorResult - to pass back the (obviously) result of validation. This will contain a status and (default) message for the failure. Then, can do action check can either interpret it themselves, or use a common method which will do it for them (by adding the message to the messages list, in case the validation had failed). This will look something like this: boolean isValid = validate(SomeValidator.validateSomething(param1, param2, ...)); This will allow the validator class to be unaware of how the command actually handles the message, while at the same time keep the code clean & DRY.
... }
...
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 agree that this is a problematic use of inheritance, and we should think of another way to model command classes internally that is not an abuse of OOP.
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.
+1 Exceptions are for unpreditable cases where you really son't have much choice what to do and should probably stop dead in your tacks, while validation is actually just asking "hey, is this thing ok or not?".
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; }
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.
----- 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
On 01/17/2012 03:14 PM, Ayal Baron wrote:
----- 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!
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
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. 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 don't necessarily see this as a cost in the same way that you do. My focus is to make the code readable and easy to understand. One of the main methods in a command class that readers are going to focus on is canDoAction. Imo in most cases the reader is going to want to know what the conditions are for success not the details of how they are all determined. Any noise in that method that detracts from this (try/catches, nested ifs, local booleans that get set and checked a lot) detract from the readability of the code. I would much rather use guard clauses in the detail method where people are less likely to be reading. Guard clauses also don't detract from readability much since they tend to be one liners at the top of methods. They are a common pattern so readers are generally not surprised to see them. My general approach is to keep the top-level methods as clean as possible and push the details down. This makes the top-level methods readable and allows interested readers to drill down only where they are interested.
I on purpose did not call it 'actionAllowed' to stress the fact that it's a global var (another thing I don't like).
I don't think global is really the right word since it would be an instance variable not a static one. It is unclear to me why you don't like instance variables. An instance of a command class represents a single attempt at running a command. It ability to succeed or fail is an instance-level concept so why not store it as an instance variable that can then be used as shared state across multiple methods.
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.
Not sure what you mean by this. Can you elaborate?
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
This is a really interesting discussion! Though I'm pretty new to oVirt, here (inline) are my two cents :) On Tuesday 17 January 2012 07:07 PM, Jon Choate wrote:
----- 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!
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
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. 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'm not sure if all validation failures can be called as "expected part of the workflow". If the caller didn't pass right parameters, why not consider it as an "unexpected" event and throw a "Validation Exception" ?
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; }
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.
This is true. In general checked exceptions and consequent try/catch blocks add a lot of noise to the code, and in many cases result in repetitive code and/or unintentional eating up of exceptions (empty or just-log-the-exception kind of catch blocks that happen in the frenzy of writing too much code in too little time). IMO, a good approach is to throw a ValidationException (which extends from RuntimeException) from validation methods, and handle this at framework level (not inside the "canDoAction" method). That way, the main validation method will look clean, and all validation errors will be handled at a central place in a consistent way. This would also mean that instead of a "boolean canDoAction", we have a "void validateActionParams", something like: public void validateActionParams() { validate1(); validate2(); validate3(); } public void validate1() { if(somethingIsWrong) { throw new ValidationException(withAllRelevantInfo); } } ... There can be another variant of the ValidationException that takes a "list" of validation errors, thus capturing multiple validation issues.
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
-- Regards, Shireesh
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@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
----- Original Message -----
From: "Moti Asayag" <masayag@redhat.com> To: engine-devel@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@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
On Tue 17 Jan 2012 10:05:15 AM IST, Omer Frenkel wrote:
----- Original Message -----
From: "Moti Asayag"<masayag@redhat.com> To: engine-devel@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@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
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
participants (9)
-
Ayal Baron -
Jon Choate -
Livnat Peer -
Maor -
Mike Kolesnik -
Moti Asayag -
Omer Frenkel -
Roy Golan -
Shireesh Anjal