6
\$\begingroup\$

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?

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Naming your function Foo() makes this borderline hypothetical code. Hypothetical code would be off-topic for Code Review, but I've allowed the question to stay open since Init(), SetTemperature(), and SetVoltage() seem plausible enough. \$\endgroup\$ – 200_success May 14 '14 at 13:39
7
\$\begingroup\$

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
\$\endgroup\$
4
  • 3
    \$\begingroup\$ Only if ERROR_SUCCESS evaluates to 0. A reasonable assumption, but you can't be sure. Better be explicit. \$\endgroup\$ – vnp May 13 '14 at 23:24
  • \$\begingroup\$ @vnp: You are right, there is actually no such guarantee in the standard, just checked. Still, it's a pretty bent implementation that does anything else (None known to me). \$\endgroup\$ – Deduplicator May 13 '14 at 23:38
  • \$\begingroup\$ Yes ERROR_SUCCESSis equal to 0 in my case \$\endgroup\$ – zadane May 14 '14 at 2:14
  • 1
    \$\begingroup\$ Rather than uncertainty over the value of ERROR_SUCCESS, I find including the comparison makes it clearer to a reader why there's an early return. "Oh, Init returns an error code; on failure we return that." And that's reason enough to be explicit. \$\endgroup\$ – Michael Urman May 14 '14 at 12:08
5
\$\begingroup\$

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

\$\endgroup\$
4
\$\begingroup\$

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);
}
\$\endgroup\$
4
\$\begingroup\$

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;
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Suggestions: 1) The ChainOfCallsHandler constructor should take an optional parameter for the constant to be interpreted as success. 2) Make register_function_call() return a reference to this so that it is chainable. With a bit of renaming, you could write SuccessChain chain(ERROR_SUCCESS).then(Init).then([]() { return SetTemperature(30); }).then([]() { return SetVoltage(5); }); \$\endgroup\$ – 200_success May 14 '14 at 13:49
  • \$\begingroup\$ seems a little over engineered. \$\endgroup\$ – craftworkgames May 15 '14 at 4:23
  • \$\begingroup\$ @craftworkgames The issue is debatable. I would agree with you if this needs to be done only once in a project. But if the OP has a lot of those call chains, I think avoiding multiple handwritten if statements is worth the effort. \$\endgroup\$ – Massimiliano May 15 '14 at 4:39
  • \$\begingroup\$ @Massimiliano That's a fair point but the question specifically says "I want my code to be nice and neat and easy to read". If I saw a ChainOfCallsHandler in a project it would take at least 20 minutes to figure out what it does and why it exists only to find it'a replacing some simple if statements. \$\endgroup\$ – craftworkgames May 16 '14 at 2:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.