Best practice for compute the function return value

Often I built functions, in C, that checks some parameters and return an error code.

Which is the best approach to stop the values checking when I found an error?

First example:

ErrorCode_e myCheckFunction( some params )
{
  ErrorCode_e error = CHECK_FAILED;

  if( foo == bar )
  {
     if( foo_1 == bar_1 )
     {
        if( foo_2 == bar_2 )
        {
           error = CHECK_SUCCESS;
        }
     }
  }

  return error;
}

Second Example:

ErrorCode_e myCheckFunction( some params )
{
  if( foo != bar )
  {
     return CHECK_FAILED;
  }

  if( foo_1 != bar_1 )
  {
     return CHECK_FAILED;
  }

  if( foo_2 != bar_2 )
  {
     return CHECK_SUCCESS;
  }
}

I prefer the first approach because I read that the MISRA rules avoid multiple return statement.

Which is the best approach?

Solution 1:

The second is best because it is so much easier to read, scales well with increased complexity and immediately stops executing the function upon errors. This is the only sensible way to write such functions when you have extensive error handling inside a function, for example if the function is a parser or protocol decoder.

That MISRA-C disallows multiple return statements in a function is a defect of MISRA-C. The intention is supposedly to disallow spaghetti code that returns from all over the place, but dogmatically banning multiple return statements can actually turn code far less readable, as we can see from your example. Imagine if you needed to check 10 different errors. You'd then have 10 compound if statements, which would be an unreadable mess.

I have reported this defect several times to the MISRA committee but they have not listened. Instead, MISRA-C just blindly cites IEC 61508 as source for the rule. Which in turn only lists one questionable source for this rule (IEC 61508:7 C.2.9) and it's some a dinosaur programming book from 1979.

This is not professional nor scientific - both MISRA-C and IEC 61508 (and ISO 26262) should feel ashamed over (directly or indirectly) listing subjective nonsense from 1979 as their only source and rationale.

Simply use the second form and raise a permanent deviation against this defect MISRA rule.

Solution 2:

I agree with the Lundin’s answer but I would like to provide another solution that complies with the single exit rule and still is similarly readable to the second example:

ErrorCode_e myCheckFunction( some params )
{
  ErrorCode_e error = CHECK_FAILED;

  if( foo != bar )
  {
     error = CHECK_FAILED;
  }
  else if( foo_1 != bar_1 )
  {
     error = CHECK_FAILED;
  }
  else if( foo_2 != bar_2 )
  {
     error = CHECK_SUCCESS;
  }
  else
  {
     // else (even empty) is required by MISRA after else-if
  }
  return error;
}

Since there are only two options in the example, we could use just one condition:

ErrorCode_e myCheckFunction( some params )
{
  ErrorCode_e error = CHECK_FAILED;

  if( (foo == bar) && (foo_1 == bar_1) && (foo_2 != bar_2) )
  {
     error = CHECK_SUCCESS;
  }

  return error;
}

This case can be even more simplified, we don’t need any local variables:

ErrorCode_e myCheckFunction( some params )
{
  return ( (foo == bar) && (foo_1 == bar_1) && (foo_2 != bar_2) )
      ? CHECK_SUCCESS : CHECK_FAILED;
}