- Is there a private variable, let's call it x that is private to the function that is being set within each (or at least a majority) of the if/case legs whose value is a method call?
- Is x is being returned at the end of the method?
- You got nested conditionals?
Here is what it would would like:
Notice the:
- result private variable at the beginning of the function
- result is being returned at the end of the function
- Nasty nasty nasty nested conditional that sets result to the value of a method
If the above is true or the code you are refactoring looks similar to this, you might want to look into using guard clauses. Sounds more complicated than it really is. Basically see if you can refactor it like so:
- Remove the private variable thats being set during each if/else leg.
- Return the method or the condition that is setting it.
Simple as that. Here is the code above refactored:
Notice now:
- No more nested if's
- After each if/else leg, we are returning the method.
Given that some legacy methods you are trying to refactor are not as trivial as this and each have their own unique level of suck - this is always a good start. This keeps your method simple, less complex and highly testable because each execution path is linear. Guard clauses baby, a good alternative to the strategy pattern.
HTH
-Mick
5 comments:
Why not a case statement?
@Anonymous - not sure what you mean, elaborate?
In your example all of your conditionals are testing the same variable. In which case, you could use a switch:
switch ( variables.type ) {
case "something cool":
result = doSomethingCool();
break;
case "something else":
return doSomethingElse();
break;
case "some other thing":
return someOtherThing();
break;
default:
return someDefault();
}
I should have made the first option a "return" to stay consistent. As an aside, however, I prefer the use of the intermediate variable so that I only have one result at the end of my method. Personal preference perhaps.
@Steve-
You could use a case, whatever your preference is. The important thing to take away from this example is not the whether to use an if or a case because both would work. I agree to remove the first variable assignment and simply return the method, this helps with code readability and in your example above would throw an error.
The important thing to take away is that you if you find yourself setting a variable in your if/case and returning it at the very end, try to see if you can instead return the method altogether and rid methods of variable assignments. It removes ambiguity and is more testable that way.
Post a Comment