2

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.

  • related (possibly a duplicate): How to do a clean refactoring of an If Else Code without leaving any free blocks? – gnat Jul 29 '18 at 8:01
  • 5
    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. – Robert Harvey Jul 29 '18 at 14:16
  • @RobertHarvey Please post as an answer and I'll accept it. – Ehtesham Hasan Jul 29 '18 at 17:04
  • 2
    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. – Simon B Jul 30 '18 at 15:48
4

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.

  • 3
    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! – Aphton Jul 29 '18 at 7:32
  • 1
    @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. – nikhil jhaveri Jul 29 '18 at 7:36
  • 1
    Well, you put it into the else-branch too. So, that's different code. – Deduplicator Jul 30 '18 at 18:44
4

Introduce a special condition handler function:

doSomething();

handleSpecialConditions();

doSomethingElse();

...

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

   doSomethingCommon();
}
  • 1
    That's the best of the bunch yet. – Deduplicator Jul 30 '18 at 18:47
  • 1
    That may be so but it's still not very good. – immibis Jul 31 '18 at 4:50
1

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();
    }
}
  • If it came to inheritance, I'd prefer to see the template pattern. Something like this. – Andy Jul 29 '18 at 10:32
  • hmmm.. that seems super bad to me. but we do have limited data to go off, why dont you write a full answer? – Ewan Jul 29 '18 at 10:47
  • 1
    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. – Andy Jul 29 '18 at 14:23
  • 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. – Flater Jul 30 '18 at 11:08
  • @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' – Ewan Jul 30 '18 at 11:14
0

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();
  • 2
    This is less readable than the original code. – immibis Jul 31 '18 at 4:50
  • @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. – Deduplicator Jul 31 '18 at 12:18
-1

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.

  • 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 Jul 29 '18 at 14:15
  • ok, but then shouldnt it be doSomethingCommon which calls doSomethingSpecial? – Ewan Jul 29 '18 at 16:30
  • @Ewan please see my next update – Timothy Truckle Jul 29 '18 at 17:00
  • 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? – Ewan Jul 29 '18 at 17:06
  • 1
    This is clearly far more confusing and unreadable than the original code. – 17 of 26 Jul 30 '18 at 15:46
-3

Boolean checks are not expensive:

doSomething()

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

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

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

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.