MISRA C 2012 Rule 15.4 There should be no more than one break or goto statement used to terminate an any iteration statement

Question

I am trying to get rid of multiple break and goto statement in my code. As the rule suggests we should not use more than one break or goto statement in any iteration statement

Sample code:

 for(int32_t i = 0; i < 10; i++) { 
  if (i == number1) {                
      return_val = 2*number1 + number2; 
      break;
  }

  number1 += (5 * number2 + 2*number3);

  if (i == number2) {
      return_val = number1 + (3 * number3);
      break;                  
  }

  number1 += 2 * number3;

  if (i == number3) {
      return_val = number1 + (2 * number2);
  }
}

I have tried with nested if statements but it is not a solution to terminate the loop.

And also instead of break, if goto statement is there what could be the fix.

Sample code with goto:

for(int32_t i = 0; i < 10; i++) { 
  if (i == number1) {                
      return_val = 2*number1 + number2; 
      goto LABE1;
  }

  number1 += (5 * number2 + 2*number3);

  if (i == number2) {
      return_val = number1 + (3 * number3);
      goto LABE2;                  
  }

  number1 += 2 * number3;

  if (i == number3) {
      return_val = number1 + (2 * number2);
  }
}

Answer 1

I think 15.4 and 15.5 are two extremely controversial MISRA suggestions. Keep in mind that they're not required but advisory then you may classify them (if your organization approves) as "disapplied" (tnx Andrew). In my opinion following those rules you will make your code more verbose, harder to read and then also more error prone.

As I said it's just my opinion, let's try a step-by-step approach. I'm not using proper names but you MUST pick good names from your domain.

Flag and keep a single break

You can use a simple _Bool flag. Assuming you included stdbool.h:

bool flag = false;

for(int32_t i = 0; i < 10; i++) { 
  if (i == number1) {                
      return_val = 2*number1 + number2; 
      flag = true;
  } else {
      number1 += (5 * number2 + 2*number3);

      if (i == number2) {
          return_val = number1 + (3 * number3);
          flag = true;
      } else {
          number1 += 2 * number3;

          if (i == number3) {
              return_val = number1 + (2 * number2);
          }
      }
    }

    if (flag)
        break;
}

Please pick a better name for flag, ideally you should describe the condition. As you can see we have a single break but we paid a huge price in legibility.

Flag and replace break with continue

What can we do? Replace break with continue and use the flag in the loop condition:

bool flag = true;

for(int32_t i = 0; flag && i < 10; i++) { 
  if (i == number1) {                
      return_val = 2*number1 + number2;
      flag = false;
      continue;
  }

  number1 += (5 * number2 + 2*number3);

  if (i == number2) {
      return_val = number1 + (3 * number3);
      flag = false;
      continue;
  }

  number1 += 2 * number3;

  if (i == number3) {
      return_val = number1 + (2 * number2);
  }
}

Slightly better but the truth is that we're not address the main issue in this code.

Move it to a separate function

If this code snippet is part of a bigger function then the truth is that we're addressing the wrong issue. It's not the break per se that can cause problems but its usage in a bigger context. In C we do not have std::optional<T> then we may use an out parameter (but it's not the only technique to achieve this result):

for(int32_t i = 0; < 10; i++) {
    if (foo(i, &number1, number2, number3, &return_value)) {
        break;
    }
}

With a separate function similar to our first implementation with flag.

bool foo(int32_t i, int32_t* number1, int32_t* return_val) {
  bool has_result = false;

  if (i == *number1) {                
      *return_val = 2 * *number1 + number2; 
      has_result = true;
  } else {
      // ...
  }

  return has_result;
}

Even better, if you can ignore 15.5 then this function will be very easy to write and to read:

if (i == *number1) {                
    *return_val = 2 * *number1 + number2; 
    return true;
}

// ...

Don't forget to add const where appropriate. I'm pretty sure that this can be refactored to be much better (too many function arguments, mixed values & pointers, too big coupling between those two functions) but with dummy names and lack of surrounding code it's hard to suggest anything better; what I want to highlight is the point to move the code to a separate function.

If code inside branches is complex enough then you may even have more benefits to introduce three separate functions. What 2 * *number1 + number2 is? What it is calculating?

Feel free to post your full code on Code Review (including surrounding code and with real names, explaining its purpose).

Answer 2

This is quite an obscure algorithm. In practice, that's usually what many MISRA rules boil down to, they block you from writing strange code. Though I can't know for certain, it smells like this code relies on various global variables and there might be better ways to write it.

However, if the algorithm is exactly like you wrote it and there's no way around it, then it is what it is. In that case I would rewrite the code with multiple return statements, which violates another (advisory) MISRA rule. But it is better to deviate from that rule than the somewhat more sound rule against having multiple break/goto.

for(int32_t i = 0; i < 10; i++) { 
  if (i == number1) {                
      return 2*number1 + number2; 
  }

  number1 += (5 * number2 + 2*number3);

  if (i == number2) {
      return number1 + (3 * number3);
  }

  number1 += 2 * number3;

  if (i == number3) {
      return number1 + (2 * number2);
  }

This is more readable than any of the alternatives in the question.

(Please note that you must also u suffix all integer constants for MISRA compliance)