Consolidate Duplicate Conditional Fragments
Problem:
- You find the exact same series of lines at the beginning or end of every branch of a conditional statement.
Solution:
- Move the common code out.
Variations:
- It may be necessary to rearrange lines to get common code up or down to the end of each block, so it can be refactored. (Example: Tests in generated SQL, different between the branches, can often be moved to the end of each SQL statement, allowing common code to be factored out.)
- It's even possible to factor out common looping logic. (See below.)
References:
- MartinFowler's RefactoringImprovingTheDesignOfExistingCode, page 243: http://www.refactoring.com/catalog/consolidateDuplicateConditionalFragments.html
Factoring out common looping logic:
From...
If Then
Unique Prefix 1
For
Unique Loop Body 1
End For
Unique Postfix 1
Else
Unique Prefix 2
For
Unique Loop Body 2
End For
Unique Postfix 2
End If
To...
tempBoolean =
If Then
Unique Prefix 1
Else
Unique Prefix 2
End If
For
If Then
Unique Loop Body 1
Else
Unique Loop Body 2
End If
End For
If Then
Unique Postfix 1
Else
Unique Postfix 2
End If
(ContributorsList: JeffGrigg, SvenDowideit)
This pattern seems to occur all the time in web-apps that I've written. My experience says it's vastly superior to refactor as follows:
commonLoopingStructure(..., body) is
for do
...
body(...)
...
end
end
uniqueLogic_1(...) is
...
end
uniqueLogic_2(...) is
...
end
refactoredProcedure(...) is
if then
uniquePrefix_1
commonLoopingStructure(..., &uniqueLogic_1)
uniqueSuffix_1
else
uniquePrefix_2
commonLoopingStructure(..., &uniqueLogic_2)
uniqueSuffix_2
end
end
DontRepeatYourself applies; replicating the if statement, even with a temp-variable, still results in error-prone redundancy that adds nothing to program comprehension, and actually makes maintenance substantially harder.
If your language supports nested procedure definitions, this becomes substantially easier to maintain still:
refactoredProcedure(...) is
uniqueLogic_1(...) is
...
end
uniqueLogic_2(...) is
...
end
if then
uniquePrefix_1
commonLoopingStructure(..., &uniqueLogic_1)
uniqueSuffix_1
else
uniquePrefix_2
commonLoopingStructure(..., &uniqueLogic_2)
uniqueSuffix_2
end
end
And, finally, if your language supports proper closures or their equivalent (e.g., Java anonymous classes), it becomes a trivial matter to factor common looping structures that hardly warrants further explanation. One can find regular and extensive application of this idea in any FunctionalProgrammingLanguage.
Such duplicated code is a common symptom of CopyAndPasteProgramming: To minimize the chances of regression in existing functionality, (in the absence of adequate automated RegressionTesting, of course) the programmer copies large sections of code, and makes minor modifications to support the new conditions / new functionality.
Here's another approach, if you're using a language that lets you do this sort of thing.
define wrapped_loop(prefix,body,postfix):
prefix
for :
body
postfix
if :
wrapped_loop(prefix1, body1, postfix1)
else:
wrapped_loop(prefix1, body2, postfix2)
The "define" here could be a macro definition, in which case "prefix", "body" and "postfix" are lumps of code; or it could be a function/method definition, in which case they're closures or something of the sort.
Is this a win? I'm not sure. But it does eliminate the code duplication.
You can go further, in languages with really sophisticated macro systems: define a general-purpose "conditional-template" thing that lets you write something like
conditional_template(
condition,
{ / ;
for :
/
/
})
Of course the syntax I've used here bears little resemblance to the real syntax of any language that allows this sort of thing. And that last suggestion would, I think, produce code that would be almost unreadable if it were applied to anything non-trivial. One moral to draw is that one can take OnceAndOnlyOnce a little too far. :-)
What about having two classes distinguished by
this.prefix();
for () {
this.body();
}
this.suffix();
Prefix, body, and suffix would be implemented accordingly in both classes. This refactors away the code that is actually duplicated: The if-then-else clause!
(Of course, I would do it in Smalltalk anyway. :-))