6

The problem

I have the following simple situation popping up all over the place. A large number of requests come to the device with a function signature like this:

Err execute( const ICommandContext &context, 
         const RoutineArguments &arguments, 
         RoutineResults &results)

There is essentially a request handling server that will call this execute the function for a variety of request types that have these signatures. We have 2 return paths in the case of an error.

  1. The Err output type (consider it to be equivalent to an int) which is used to inform the server or system that something has gone wrong that is to do with the system, not the request. This is always sorted at the top of the function before the user request is dealt with.
  2. RoutineResults provides a setStatus function that can be used to return failure information of the request to the client.

For this reason we have a lot of this type of code popping up:

// Failure due to request
Err error = someFunctionCall(clientInput);
if (!error.success()) {
    results.setStatus(error); // Inform the client of the error
    return SUCCESS; // Inform the system that we are all good
}

We have a particular request type that has around 15 parameters that come in and are sent off around the system. We would conceptually need 15 of this if error do set which seems wasteful. It is also prone to errors if we need to go through and change anything about how we return. How can we effectively delegate the setStatus and return to a short amount of code that only needs to happen once in the function?

A Macro Solution

A c system might solve this with a macro, something like:

#define M_InitTry Err error
#define M_Try(statement) if (!(error = statement).success()) { goto catch_lab; }
#define M_Catch catch_lab: if (!error.successs())
#define M_Return return error

Which would be used like this:

Err execute( const ICommandContext &context, ...) {
    M_InitTry;

    ...

    M_Try(someFunctionCall(clientInput));
    M_Try(someFunctionCall(otherClientInput));
    ...

    M_Catch {
        // Other specific actions for dealing with the return.
        results.setStatus(error);
        error = SUCCESS;
    }
    M_Return;
}

This cleans the code nicely, but is not particularly nice with the goto. It will cause problems if defining variables that might be skipped by a goto.

A delegating solution

I was trying to think of a more C++ so I thought an RAII type delegate might help. Something like:

class DelegateToFunctionEnd {

    typedef std::function<void(void)> EndFunction; 

    public: 
    DelegateToFunctionEnd(EndFunction endFunction) : callAtEnd(endFunction) { }

    ~DelegateToFunctionEnd() {
        callAtEnd();
    }

    private:         
    EndFunction callAtEnd;
};

Pretty simple, it does a delegate of the action until the function return by implementing the action in the destructor. You might use it like this:

Err execute( const ICommandContext &context, ...) {
    Err error;
    DelegateToFunctionEnd del(std::bind(&RoutineResults::setStatus, &results, std::cref(error)));

    error = someFunctionCall(clientInput));
    if (error) return SUCCESS;

    ...
}

Live example. This solution seems ok, but has several problems:

  1. It is not as clear what is happening.
  2. It is easier to make a mistake about setting error correctly.
  3. You still need a large number of if statements to deal with the returns.
  4. The ability to configure the terminating action is not great.
  5. Dangerous if the user doesn't carefully consider the destruction order of items at function return.

A better solution?

This must be a problem that comes up often. Is there a general solution that provides a clean delegation of this set and returns type action?


I have some unfortunate restrictions below. Don't let these stop you from answering because it might be helpful for future people.

  1. I am working on a c++03 limited system. We have boost, but no c++11.
  2. Embedded system and we have silly rules about exceptions and memory allocation.
Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
Fantastic Mr Fox
  • 32,495
  • 27
  • 95
  • 175
  • Can you have an array of `function` and then iterate over that array with your check ? something like `for (auto&& f : functions) { auto error = f(); if (error) {/*..*/} }` ? – Jarod42 Jul 25 '18 at 12:06
  • @Jarod42 Yeah probably, it would be interesting to achieve with `boost::function` but probably very doable. You should add this as an answer. – Fantastic Mr Fox Jul 25 '18 at 12:07
  • Short answer - take a look at how is haskell `Exception` created. Basically it wraps the function and propagates return value or error depending what happend until it gets to the end/the error handler. I'll try to give more elaborate answer soon – bartop Jul 25 '18 at 12:32

4 Answers4

5

If error status codes are proving troublesome, you should consider using exceptions instead. That is, change the API of your functions

  • so they are guaranteed to have success as a post-condition
  • throw a suitable std::exception in the event of failure

It is impossible to "forget" to examine a status code if you do this. If you choose not to handle an error condition, the exception thrown by low-level code automatically percolates upwards. You only need to catch a low-level exception if

  • You need to do some manual roll-back or deallocation in the event of an error, and RAII is impractical. In this case you would rethrow the expcetion.
  • You want to translate a low-level exception message or exception type into a high-level message, using a thrown) nested exception.
Raedwald
  • 46,613
  • 43
  • 151
  • 237
  • 1
    Yeah, this would be so nice. Unfortunately its an embedded system and we have a no exceptions rule, which is not very good (especially since this is not a critical section of the code). But this is certainly a good answer to the question. – Fantastic Mr Fox Jul 25 '18 at 11:40
  • @FantasticMrFox SO, on your project you write customer allocators that are `noexcept`? Because if you don't, much of the STL will throw exceptions. – Raedwald Jul 25 '18 at 11:44
  • 1
    [How much footprint does C++ exception handling add](https://stackoverflow.com/questions/691168/how-much-footprint-does-c-exception-handling-add)? – Raedwald Jul 25 '18 at 11:45
  • I am aware of both these things and have made people aware of this. Unfortunately no change at the moment. I am also pushing for an upgrade from c++03 to c++11. But 1 person pushing can only do so much ... – Fantastic Mr Fox Jul 25 '18 at 11:48
  • @FantasticMrFox Make that two and counting. – Ron Jul 25 '18 at 12:04
1

Split the function.

The inner function returns an error code based on user input; the outer translates that to a client error, and only returns server side errors.

Inner function contains:

if(Err error = someFunctionCall(clientInput))
  return error;

repeatedly. Outer has the relay to client error code, but only once.

Err just needs an operator bool. If it cannot have it, create a type that converts to/from Err and has an operator bool.

Yakk - Adam Nevraumont
  • 262,606
  • 27
  • 330
  • 524
  • I don't think this is an answer to the question ... Do you wan't to elaborate some more? – Fantastic Mr Fox Jul 25 '18 at 12:02
  • The body of `execute` becomes `{ if (!server_validate(params...)) return FAILURE; do_execute(params); return SUCCESS; }`, where `server_validate` and `do_execute` are the top half and bottom half of your current `execute` respectively – Caleth Jul 25 '18 at 12:06
  • @Caleth then you would just be moving the big long line of `if error do` into the `server_validate` function. This hardly solves the problem, right? – Fantastic Mr Fox Jul 25 '18 at 12:09
  • @FantasticMrFox The "big long line" with the marshall from error code to one spot? There are short lines where you do `if (blah) return x;` in the inner function. – Yakk - Adam Nevraumont Jul 25 '18 at 13:21
1

Maybe, you can write your statement as array, something like:

Err execute( const ICommandContext &context, ...)
{
    const boost::function<Err()> functions[] = {
        boost::bind(&someFunctionCall, std::ref(clientInput)),
        boost::bind(&someFunctionCall, std::ref(otherClientInput)),
        // ...
    };

    for (std::size_t i = 0; i != sizeof(functions) / sizeof(functions[0]); ++i) {
        Err err = functions[i]();

        if (!err.successs()) {
            results.setStatus(err);
            return SUCCESS;
        }
    }
    return SUCCESS;
}

and if you do that several time with only different statements, you might create

Err execute_functions(const ICommandContext &context, std::function<Err()> functions);

Maybe also provide other entry points as OnError depending of your needs.

Jarod42
  • 203,559
  • 14
  • 181
  • 302
0

Can you add a method to error that does the check etc and return a bool.

if(!someFunctionCall(clientInput).handleSuccess(results))
{
    return SUCCESS;
}