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: References:

Factoring out common looping logic:

From...
   If <some-condition> Then
      Unique Prefix 1
      For <common-looping-logic>
          Unique Loop Body 1
      End For
      Unique Postfix 1
   Else
      Unique Prefix 2
      For <common-looping-logic>
          Unique Loop Body 2
      End For
      Unique Postfix 2
   End If
To...
   tempBoolean = <some-condition>

If <tempBoolean> Then Unique Prefix 1 Else Unique Prefix 2 End If

For <common-looping-logic> If <tempBoolean> Then Unique Loop Body 1 Else Unique Loop Body 2 End If End For

If <tempBoolean> 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 <common-looping-logic> do
      ...
      body(...)
      ...
    end
  end

uniqueLogic_1(...) is ... end

uniqueLogic_2(...) is ... end

refactoredProcedure(...) is if <condition> 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 <condition> 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 <common_looping_logic>:
     body
   postfix

if <condition>: 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,
   { <prefix1> / <prefix2> ;
     for <common-looping-logic>:
       <body1> / <body2>
     <postfix1> / <postfix2>
   })

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 <some-condition> and implementing like

  this.prefix();
  for (<common-loop>) {
    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]

EditText of this page (last edited January 28, 2009) or FindPage with title or text search