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.

• gnat: related (possibly a duplicate): How to do a clean refactoring of an If Else Code without leaving any free blocks?
• Robert Harvey: Calling a method multiple times in an if statement is not code duplication. The whole point of an "extract method" refactoring is to eliminate code duplication, so multiple calls to a method don't count.
• Ehtesham Hasan: @RobertHarvey Please post as an answer and I'll accept it.
• Simon B: The DRY principle has a lot to answer for. It's one line of code that's duplicated. Any alternatives are going to be uglier, less readable or more complicated than what you have here.

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.

• Aphton: If your first assumption was true, then the problem would be trivial. OP would have figured it out himself. But he did not. The problem, as it is presented, implies it is not possible!
• nikhil jhaveri: @Aphton Hence the second part of the answer. Also, as a general rule, assumptions, especially about the cost of boolean checks, should always be avoided.
• Deduplicator: Well, you put it into the else-branch too. So, that's different code.

Answer 2

Introduce a special condition handler function:

doSomething();
handleSpecialConditions();
doSomethingElse();

...

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

• Deduplicator: That's the best of the bunch yet.
• Stack Exchange Broke The Law: That may be so but it's still not very good.

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();
 }
}

• Andy: If it came to inheritance, I'd prefer to see the template pattern. Something like this.
• Ewan: hmmm.. that seems super bad to me. but we do have limited data to go off, why dont you write a full answer?
• Andy: The solution I proposed circumvents the problem of you accidentally forgetting to call the base.DoSomething() method, because it's called by the template. Because of that I like to put the necessary calls in the parent class and let the children only really define the specialisations. When it comes to the full answer, I don't feel like answering this question because the question is too broad and you'd need much more information to find the right solution. Perhaps the template pattern would be overkill in this situation, other time the if/else if is the wrong path. It depends.
• Flater: This answer assumes that doSomethingCommon() can reasonably be abstracted into a base class where A/B can represent its subtypes. As there is no inheritance present in the example, not even a mention of objects altogether, suggesting inheritance seems like a very big leap to make. I'm not saying you're wrong, I'm just saying that your answer only applies to a small subset of possible scenarios that OP's example could be taking place in.
• Ewan: @Flater it's such a generic example I don't think you can make assumptions either way. Q: 'how do I avoid conditionals?', A: 'use inheritance', Q: 'how do i avoid conditionals that have a special problem which means inheritance doesn't work?', A: '98% of the time, refactor and use inheritance'

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();

• Stack Exchange Broke The Law: This is less readable than the original code.
• Deduplicator: @immibis: If doSomethingSpecific.... is just that one call of a well-named function, sure. It could be more complicated though, defying easy naming, and being very involved with the rest of the function. The simplified example-code gives no clues either way. Sure, that's unlikely, but it's better to know the option.

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.

• Ewan: my question with this is that the calling code needs to know to call the special behaviour vs the common behaviour? The code implies that some cases are neither special A or B, how are those dealt with?
• Ewan: ok, but then shouldnt it be doSomethingCommon which calls doSomethingSpecial?
• Timothy Truckle: @Ewan please see my next update
• Ewan: maybe im just not following. Your saying that everything implements specialBehaviour, including a class which only runs the common behaviour? I dont see how this stops you forgetting to call commomBehaviour in one of these classes. Its simply a replacement of inheritance with composition and the interface should simply be IDoSomthing and make no reference to special behaviour?
• 17 of 26: This is clearly far more confusing and unreadable than the original code.

Answer 6

Boolean checks are not expensive:

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

• combinatorics: conditionA and conditionB could be functions which return a boolean. In that case evaluating the condition could be expensive.
• Jörg W Mittag: ... and non-idempotent.
• Ewan: avoiding the expense of calculating conditionX can always be done by storing the result, no matter how many times you check it
• Timothy Truckle: You code is less readable as the OPs example,
• bdsl: This removes the duplicate mention of doSomethingCommon(), but at the cost of duplicating the mentions of conditionA and conditionB