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

Unless you MUST strictly adhere to MISRA (even suggestions) do yourself a favor and **IGNORE** 15.4 and 15.5. I NEVER ever saw better code when following those two rules. It will just make everything verbose. BTW you can "fix" it using a boolean flag (and cascade of `else if`) — Adriano Repetti, Dec 10 '17 at 17:19
You can terminate the loop by setting the variable `i` to the terminating value `10`. This must also be used as a flag, so that `number1 += (5*...)` will not be executed. When the code reaches the end of loop it will see the termination condition fulfilled. -- You can also try to use `i=10; continue;` instead of `break;` Dunno if `continue` is allow by MISRA. — harper, Dec 10 '17 at 18:50
@Amardeep if you really MUST do it then declare a `_Bool` flag with a **meaningful name**. Set that flag instead of `break` and check it once at the end of the loop `if (flag) break`. Of course you also need to add `else` branches. Overall code will be (IMO) much worse and harder to read (and then error prone). I can't discuss about MISRA but if you keep your functions short (which is more important) then multiple exit points are far better than their counterpart. — Adriano Repetti, Dec 11 '17 at 08:58

Adriano Repetti · Accepted Answer · 2017-12-11T15:32:33.040

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).

No you are not "free to ignore them" - but you may reclassify them to 'disapplied' if your organisation approves. — Andrew, Dec 11 '17 at 15:17

score 0 · Answer 2 · answered Dec 11 '17 at 09:28

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)

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

2 Answers2

Flag and keep a single break

Flag and replace break with continue

Move it to a separate function

Flag and keep a single `break`

Flag and replace `break` with `continue`