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 <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]