How to avoid code duplication in if else condition?

Question

How can I avoid duplicate use of doSomethingCommon() in the following block of code?

doSomething();

if (conditionA) {
    doSomethingSpecificToConditionA();
    doSomethingCommon();
}
else if (conditionB) {
    doSomethingSpecificToConditionB();
    doSomethingCommon();
}

doSomethingElse();

NB: Calculating conditionA and conditionB can be expensive.

Answer 1

Calling doSomethingCommon() from 2 conditional blocks should be fine, and technically NOT code duplication, as only one of the conditional blocks will be executed at any given time.

However, if doSomethingCommon() is not already a separate method, you should refactor it into a method and again calling the same is NOT code duplication.

Answer 2

Introduce a special condition handler function:

doSomething();

handleSpecialConditions();

doSomethingElse();

...

void handleSpecialConditions()
{
   if (conditionA)
      doSomethingSpecificToConditionA();
   else if (conditionB)
      doSomethingSpecificToConditionB();
   else
      return;

   doSomethingCommon();
}

Answer 3

Consider using inheritance to avoid multiple conditionals in a method

public class SomethingA : Something
{
    public override void DoSomething()
    {
        base.DoSomething() //all the common things
        this.SpecialForASomethings();
    }
}

Answer 4

If you don't want to extract handleSpecialConditions() like Eternal21 suggested (you probably should), consider memoizing:

doSomething();

bool flagA;
if (((flagA = conditionA)) || conditionB) {
    if (flagA)
        doSomethingSpecificToConditionA();
    else
        doSomethingSpecificToConditionB();
    doSomethingCommon();
}

doSomethingElse();

Answer 5

You should also consider other best practices like Prefer Composition over inheritance. So my suggestion would look like this:

class CommonBehavior{
  void doSomethingCommon(){
    //...
  }
}

interface SpecialBehavoior{
  void doSomethingSpecial();
}

class SpecialBehavior1 implements SpecialBehavoior{
   @Inject
   private CommonBehavior commonBehavior;
   @Override
   public void doSomethingSpecial(){
    // what ever
        commonBehavior.doSomethingCommon();
    // what ever more
   }
}

class SpecialBehavior2 implements SpecialBehavoior{
   @Inject
   private CommonBehavior commonBehavior;
   @Override
   public void doSomethingSpecial(){
    // what ever
        commonBehavior.doSomethingCommon();
    // what ever more
   }
}

This unleashes its full power when combined with the Tell, don't ask! principle. That means that you convert your "conditions" as soon as possible to SpecialBehavior objects (usually in a Factory class that still uses ifs and/or switches) and pass them around.

---

my question with this is that the calling code needs to know to call the special behaviour vs the common behaviour? – Ewa

This is the "smart" thing about inheritance and tell, don't ask!: the calling code does not need to know. The objects passed have this knowledge.

The calling code "splits up". You create the SpecialBehavior objects as soon as the data enters your system and pass them around to places where methods of a common API are called and any special behavior is coded in the different implementations of that interface.

The code implies that some cases are neither special A or B, how are those dealt with? – Ewan

Doing "nothing" is also a "special behavior" so you need an implementation of the SpecialBehavior interface for that:

class DoNothing implements SpecialBehavoior{
   @Override
   public void doSomethingSpecial(){
        // do nothing
   }
}

This looks like a lot of "glue code" but thanksfully Java has an eye candy named lambda so that you can equally write it like this:

SpecialBehavoior doNothing = ()->{;};

---

ok, but then shouldnt it be doSomethingCommon which calls doSomethingSpecial? – Ewan

Both ways are possible and it depends on your requirements.

As far as I understood your "special behavior" is mutual exclusive. In this case it is better to have a single instance of the CommonBehavior class passed to the individual instances of SpecialBehavior implementations. Especially the "DoNothing" requirement is easy to solve this way.

If you wold do it the other way around you would have two down sides:

1. You need a separate instance of CommonBehavior as "wrapper" for each and every SpecialBehavior instance.

2. There is no Tell, don't ask! way to prevent the "common behavior" from being executed.

   This means you need something like

   if(!specialBehavior instance of DoNothing){
      // do the common behavior
   }
   

   and that is exactly the kind of code we initially tried to avoid.

Answer 6

Boolean checks are not expensive:

doSomething()

if (conditionA) {
    doSomethingSpecificToConditionA();
}
else if (conditionB) {
    doSomethingSpecificToConditionB();
}

if (conditionA || conditionB) {
    doSomethingCommon();
}

doSomethingElse()