'''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. :-) --GarethMcCaughan ----- What about having two classes distinguished by and implementing like 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. :-)) --HaskoHeinecke ----- [CategoryRefactoring]