Avoid duplicating code

Question

Hi all, let's say I have:

  switch( choice ) {
  case A:   
     stmt;
     do_stmt_related2A;
  break;

  case B:
     stmt;
     do_stmt_related2B;
  break;

  case C: something_different();
   ...
  }

How could I avoid duplicating stmt code?

But is there any workaround? gcc extension label as value looks quite good for such situation.

   switch( choice ) {
     do {
     case A:  ptr = &&A_label;
     break;
     case B:  ptr = &&B_label;
     } while(0);
              stmt;
              goto *ptr;
     case C: ...

Is there any trick that can do the same in ANSI-C? Edit: Of course I have thought of function/macro/inline. But anything else? It's not about performance either. Just for educational purpose. ;)

Answer 1

How could I avoid duplicating stmt code?

By putting it into a function and call that.

And, no, you do not know whether this will slow down your application until you profiled it and found it to be the bottleneck. (And in case it really is, use a macro or, if that's C99, make the function inline.)

Answer 2

Why don't you just refactor stmt (I'm assuming this is a big chunk of instructions rather than a single line) into its own function do_stmt() and call it? Something like:

switch( choice ) {
    case A:
        do_stmt();
        do_stmt_related2A;
        break;
    case B:
        do_stmt();
        do_stmt_related2B;
        break;
    case C: something_different();
        ...
}

That gcc trick is truly hideous. I would rather have readable code over such monstrosities any day.

You should always assume that the programmer that inherits your code is a homicidal maniac who knows where you live :-)

Answer 3

There will be some code either way - you can have duplicated code, or code to avoid duplication. So I'm curious as to how complex the stmt; code really is.

The simple, clean solution is to move the shared part (stmt) into a seperate function.

void do_shared_stmt(void) {
 stmt;
}
/* .... */
swtich(choise) {
case A:
  do_shared_stmt();
  do_stmt_related2A();
  break;
case B:
  do_shared_stmt();
  do_stmt_related2B();
  break;
case C:
  something_different();
/* ... */
}

Another solution (that might be acceptable, depending on your situation) is to nest branching statements:

swtich(choise) {
case A:
case B:
  stmt;
  if(choise == A) {
    do_stmt_related2A();
  } else {̈́
    do_stmt_related2B();
  }
  break;
case C:
  something_different();
/* ... */
}

Answer 4

I'd go for something like what I append here. Sure, you probably had the idea yourself to have a nested switch statement, nothing new. What this does in addition is that it evaluates choice only once.

It also avoids the gcc construct of addresses of labels, so there is no indirection, here. A decent compiler should be able to optimize such a thing quite well.

Observe also, that my_choice is an intmax_t so this should be compatible with any type choice could have.

(Putting in the for is just for the fun, and would only work with C99, obviously. You could replace it with an extra {} around the stuff and just a declaration of my_choice for C89.)

typedef enum { one = 1, two, three } number;

int main(int argc, char** argv) {

  number choice = (number)argc;

  for (intmax_t _0 = 0, my_choice = choice; !_0; ++_0)
    switch(my_choice) {
    case one:;
    case two:;
      {
        printf("Do whatever you want here\n");
        switch (my_choice) {
        case one:
          printf("test 1\n");
          break;
        case two:
          printf("test 2\n");
          break;
        }
        printf("end special\n");
      }
      break;
    default:;
      printf("test %d, default\n", choice);
    }
}

Answer 5

Using goto pointers will likely result in slower code because it shuts off some of gcc's other optimizations (or would the last time I read up on it). Gcc essentially decides that it could be way too complicated to try to keep up with what might be going on and assumes that many more branch instructions can target every goto label that has been && than is really the case. If you insist on trying to use this method I suggest that you attempt to use an integer and another switch/case rather than the goto. Gcc will like be able to make sense of that.

Aside from that, for many statements this may not be worth the work or it may actually work better the way it is. It really does depend a lot on what stmt actually is.

Refactoring stmt into a static function will likely yield good results if stmt is indeed expensive or big code.

Another thing you might try if you can would be to hoist stmt out of the switch/case and just execute always execute it. Sometimes this is the cheapest way to go, but that really does depend on what stmt actually does.

Another thing you might do is refactor all of stmt , do_stmt_related2A , and do_stmt_related2A all into file static functions, like this:

// args in this is just a psudocode place holder for whatever arguments are needed, and 
// not valide C code.
static void stmt_f(void (*continuation)(arg_list), args) {
   stmt; // This corresponds almost exactly to stmt in your code
   continuation(args);
}
static void do_stmt_related2A_f(args) {
    do_stmt_related2A;
}
static void do_stmp_related2B_f(args) {
    do_stmt_related2B;
}

...
    switch (condition) {
       case A:
          stmt_f(do_stmt_related2A_f, args);
          break;
       case B:
    ...

The call to the continuation function at the end of stmt_f is a tail call and will most likely become a jump rather than a real call. Because all of these are static it is possible that the compiler would see the entire set of values which could be continuation functions and optimize some more, but I don't know that.

Unless stmt is very big it is very likely that this is a micro-optimization that just isn't worth it. If you really want to know then you should compile to assembly and try to see what the compiler really does with your original code. It may very well do a better job than you think on it.

Oh, one last thing you might try is if you control what actual values A, B, C.. can take then you could make sure that the ones with similar prefixes have adjacent values. If A and B really are next to each other and if the compiler decides that it needs to break up the switch/case into a few different jump tables then it will likely put A and B in the same jump table and also see that they have the same prefix and hoist that code out for you. This is more likely if C, which does not share that prefix, is not adjacent to A or B, but then your overall code could be worse.

Answer 6

You simply need two control structures. One for dictating execution of the first order instructions, a second for the second order instructions.

switch (state) {
    case A:
    case B:
        stmt;

        switch (state) {
            case A:
                do_stmt_related2A;
                break;
            case B:
                do_stmt_related2B;
                break;
        }

        break;

    case C:
        something_different();
        ...
}

It's also worth noting that switch is less suitable for the second order control structure, you likely want to use a more traditional conditional branch. Lastly, the real answer to your question regarding goto-label-pointer is that this is what subroutine linkage is for. If your instructions are more complex than a single expression, then you can and should use a function.

Answer 7

I'll probably do something like that :

void do_stmt(int choice)
{
    stmt;
    switch(choice)
    {
         case A:
             do_stmt_related2A;
             break;
         case B:
             do_stmt_related2B;
             break;
    }  
}
/* ... */
switch(choice)
{
    case A:
    case B:
        do_stmt(choice);
        break;
    case C:
         something_different();
...

Answer 8

Here's one alternate to function calls or secondary switch statements:

isA=false;
switch( choice ) { 
  case A:    
    isA=true;
  //nobreak
  case B: 
    stmt; 
    isA ? do_stmt_related2A : do_stmt_related2B;
  break; 
  case C: 
    something_different(); 
  break;
} 

However, I can't say I really recommend it as a coding style.

Answer 9

Aside from the common sentiment to refactor the common code into a sub-function, the simplest way to refactor your code using standard C functionality is probably:

if (choice == A || choice == B) {
    stmt;
}

switch( choice ) {
  case A:   
    do_stmt_related2A;
    break;

  case B:
    do_stmt_related2B;
    break;

  case C:
    something_different();
    ...
}

It cleanly expresses what you want to do, and it avoids the switch-inside-a-switch that prevents some compilers from optimizing the code effectively.

Answer 10

If you're using gcc, one option is nested functions. These are functions that have access to all the variables of the parent function.

For example:

void foo(int bar) {

    int x = 0;
    void stmt(void) { //nested function!!
        x++;
        if (x == 8) {
            x = 0;
        }
    }

    switch( bar ) {
    case A:   
        stmt();
        do_stmt_related2A;
    break;

    case B:
        stmt();
        do_stmt_related2B;
    break;

    case C:
        something_different();
        ...
    break;
    }
}

Since this is a gcc extension, it should obviously not be used for code intended to be portable. But it can be fun to play with :) and in certain circumstances makes the code much more readable.

If you have an executable stack you can even create a pointer to a nested function. It's a lot like a lambda function, except horrible - or horribly fun, depending on your perspective. (Like all pointers to "local" objects, the pointer becomes invalid when the parent function exits! Beware!)

Answer 11

Go back to the original meanings of the different case-values. What do cases A, B and C represent? If A and B should execute partially the same code, they should have some similarities. Find them and express them in another way.

The following example makes this clearer. Suppose that A, B and C are 3 different kinds of animals, and the duplicated code for A and B is actually typical for animals that can fly. You could then write your code like this:

if (can_fly(choice))
   {
   stmt;
   }

switch( choice )
  {
  case A:   
     do_stmt_related2A;
     break;
  case B:
     do_stmt_related2B;
     break;
  case C:
     something_different();
     break;
  }

That way, if a 3rd choice D is added later to your application, the chances of forgetting the "stmt" duplicated code is slightly smaller.

If you really want to prevent a function call (to can_fly), and A, B and C are numerical constants, you could make use of bitmasks. In this example, we use one bit to indicate the animal can fly:

if (choice & BIT_CAN_FLY)
   {
   stmt;
   }

switch( choice )
  {
  case A:   
     do_stmt_related2A;
     break;
  case B:
     do_stmt_related2B;
     break;
  case C:
     something_different();
     break;
  }

Answer 12

You can easily just put your stmt code into a function and call that function from your case or wherever, you can call a function how ever many times you want.

Answer 13

i see only two possible arrangements of ur logic.

L1:
 val
 |
 |-------------------
 |        |         |
 A        B         C
 |        |         |
 stmt     stmt      crap
 |        |
 Afunk    Bfunk

L2:
 val
 |
 |-------------------
 stmt               |
 |---------         |
 |        |         |
 A        B         C
 |        |         |
 Afunk    Bfunk     crap

In L1, use inline function for stmt or use L2 if branching twice is ok. If you are talkin about src code duplication(as opposed to duplication in the binary), just inline func will solve ur problem.

Answer 14

Others have already given this answer, but I wanted to state it in formal terms.

If performance isn't your concern, then you've correctly identified one of the typical "code smells" which is duplicate code.

One of the most basic refactorings is called Extract Method. Learn it, live it, love it. The process of identifying duplicate code and eliminating it should be part of your minute-by-minute workflow.