Checking errors from a series of function calls

Question

I have to work with C-style functions which return ERROR_SUCCESS when successful or an error code when not. I want my code to be nice and neat and easy to read but I am not sure I like what I have.

Option one:

int Foo()
{
    int result = Init();

    if (result == ERROR_SUCCESS)
    {
        result = SetTemperature( 30 );
    }
    else
    {
        return result;
    }

    if (result == ERROR_SUCCESS )
    {
        result = SetVoltage( 5 );
    }
    {
        return result;
    }

}

Option two:

int Foo()
{
    int result = Init();

    if (result != ERROR_SUCCESS)
        return result;

    result = SetTemperature( 30 );

    if (result != ERROR_SUCCESS)
        return result;

    result = SetVoltage( 5 );

    if (result != ERROR_SUCCESS)
        return result;

}

Please note that I can't use exceptions here. If there is an error then I simply need to return from the function with that error. Is there an even better way to organize this code?

Answer 1

Do your early returns properly, and there will be no more to tweak:

int Foo()
{
    int result;
    if((result = Init()))
        return result;
    if((result = SetTemperature( 30 )))
        return result;
    return SetVoltage( 5 );
}

Using the extra parentheses () around the assignment to suppress a compiler warning.

I don't check for expr != ERROR_SUCCESS because that's equivalent to expr in a boolean context.

If there are really a lot of those calls, consider investing in a preprocessor macro.
Don't forget to undefine it after use if the compilation unit is not at an end.
Also, many really don't like those.

#define OkOrReturn(a) do{int result = a; if(result) return result;}while(0)
#undef OkOrReturn

Answer 2

If you don't mind I'll be using SUCCESS instead of ERROR_SUCCESS, I find it more clear.

What is commonly accepted as a clearer solution, is to have a single return point and proceed to execution only when a sequence of calls is successfull; Small tweaks to your code (or other code posted here) could achieve this :

int Foo()
{
    int result;
    if (SUCCESS == (result = Init()))
    {
        if (SUCCESS == (result = SetVoltage(5)))
        {
            if(SUCCESS == (result = SetTemperature(30)))
            {
                result = SetVoltage(5);
            }
        }
    }
    return result; // Single return point
}

Note, another tweak was the yoda expressions :

We put the unmodifiable r-value SUCCESS to the left of the expression to avoid involuntary assignments. In such long chains of checks a == may be miswritten as = and then all hell breaks loose. If you have your r-value to the left though this would cause a compilation error and it couldn't go unnoticed.

Now, the same technique has a more compact way to be written (it takes getting used to it thougt)

int Foo()
{
    int result;
    if (SUCCESS == (result = Init()) &&
        SUCCESS == (result = SetVoltage(5)) &&
        SUCCESS == (result = SetTemperature(30)))
    {
        result = SetVoltage(5);
    }
    return result; // Single return point
}

and yes it works with the expected precedence due to short circuiting in the evaluation of the truth value context in if. So I use this when I have no extra code to call between the function calls themselves

Answer 3

Your second function is better, except that it fails to return ERROR_SUCCESS if all three operations succeed. That should have triggered a compiler warning. (You do compile with warnings enabled, right?)

Checking result codes is very common in C code. I make the following recommendations:

• Every checkable function call should be in a conditional.
• Some libraries are designed to return 0 on success, while others return 0 on failure. Therefore, make the comparisons explicitly using == or !=; don't use ! or implicit interpretation of the result code as a boolean value.
• Use Yoda conditions: put the constant at the front of the conditional. This has four benefits:
  • The error-checking idiom is immediately recognizable.
  • The constant is easy to understand, so putting it in front reduces mental workload. Putting it at the end would create suspense, as you have to read the whole line, which is more complex, before you see the constant you are comparing against.
  • You can't mistakenly type = instead of == or !=, since the constant is not a valid lvalue.
  • The character columns line up nicely vertically.

My suggested way to write your function:

int Foo()
{
    int status;

    if (ERROR_SUCCESS != (status = Init()))             return status;
    if (ERROR_SUCCESS != (status = SetTemperature(30))) return status;
    return                status = SetVoltage(5);
}

That unconventional formatting works, I think, because the function is so simple. The final status= assignment is superfluous; consider it to be a kind of comment.

It could also be written with more conventional formatting:

int Foo()
{
    int status;

    if (ERROR_SUCCESS != (status = Init())) {
        return status;
    }
    if (ERROR_SUCCESS != (status = SetTemperature(30))) {
        return status;
    }
    return status = SetVoltage(5);
}

Answer 4

As you are using C++, I would try to devise a class that handles a chain of calls the way you want. A toy example may be:

#include<vector>
#include<functional>
/**
 * @brief Executes a chain of calls in the same order they were registered.
 * Stops if some of the call returns an error.
 */
class ChainOfCallsHandler {
public:
   using callback_type = std::function< int ()>;

   void register_function_call(callback_type f) {
       m_list.push_back( f );
   }

   int execute() {
       int result = 0;
       for(auto& x : m_list) {
           result = x();
           if ( result != 0) break;
       }
       return result;
   }
private:
   std::vector< callback_type > m_list;
};

This class may be used by clients in this way:

#include<ChainOfClassHandlers.h>
#include<iostream>

int foo() {
    std::cout << "Executing " << __func__ << std::endl;
    return 0;
}

int bar(float a) {
    std::cout << "Executing " << __func__ << std::endl;
    return 1;
}

struct foobar {
    int operator()() {
      std::cout << "Executing " << __func__ << std::endl;
      return 0;        
    }
};

int main() {

    ChainOfCallsHandler handler;

    handler.register_function_call( foo );
    handler.register_function_call( [](){ return bar(1.0); } );
    handler.register_function_call( foobar() );

    std::cout << handler.execute() << std::endl;
    return 0;
}

You may experiment with a live example here.

---

A slight modification of the class (building upon the suggestions in the comments):

class ChainOfCallsHandler {
public:
   using callback_type = std::function< int ()>;

   ChainOfCallsHandler(int success ) : m_success_value(success) {}

   ChainOfCallsHandler& operator()(callback_type f) {
       m_list.push_back( f );
       return *this;
   }

   int execute() {
       int result = m_success_value;
       for(auto& x : m_list) {
           result = x();
           if ( result != m_success_value) break;
       }
       return result;
   }


private:   
   int m_success_value;
   std::vector< callback_type > m_list;
};

inline ChainOfCallsHandler chain_calls_with_success_value(int success_value) {
    return ChainOfCallsHandler(success_value);
}

may add a little bit of syntactic-sugar in client code:

int main() {

    auto handler = chain_calls_with_success_value(0)
    ( foo )
    ( [](){ return bar(1.0); } )
    ( foobar() );

    std::cout << handler.execute() << std::endl;
    return 0;
}