Return NULL in case of success

Question

This question is a clearer version of a question I posted on SO.

I have a C++ Planner object with a method that computes a Route from a start point to a destination point. Planner is the owner of the computed Route pointer.

Now, I have a second method (call it TestRoute), that takes the route object and tells if the destination is reachable from the start point, and if not, tries to find a route that passes through fuelling stations.

The original implementation of TestRoute was to return a Route pointer when the trip is feasible (with or without refuelling), and NULL in case of failure:

• if a refuelling is necessary, TestRoute returns a pointer to the new Route;
• if no refuelling is necessary, it returns a pointer to a copy of the original Route, since its owner is Planner.

In these both cases, it is up to the caller of TestRouteto destruct the returned Route object.

Well, now, a colleague of mine wants the method returns NULL when the route is feasible without refuelling, to avoid useless copy and destruction.

So the method would return:

• NULL in case of failure
• a pointer to a Route in case of success when a new Route is computed
• NULL in case of success (!!) when no refuelling is necessary.

It sounds to me like a terrible design. May I have some opinion?

Answer 1

Returning NULL in case of success (the most simple case for success at that) is definitely contrary to what most people will expect.

Returning a pointer that the user has to manually destruct isn’t super great either. I’d suggest using C++11 unique_ptr but using C++11 may not be feasible in your case.

One thought I had is to make TestRoute private and have the Planner call it whenever it computes a new route.

If the test fails, return NULL, otherwise return the route.

What’s nice about this approach is that you can implement TestRoute however you (or your colleague) please, and the user of the class won’t need to know the details of how it is implemented. The user will just ask for a route from point A to point B and will be guaranteed it’s a valid route with refueling points so long as they don’t receive NULL.

You could also split your method into a few different methods if the performance hit isn’t too great.

For example, for TestRoute, have it return true if the route is possible, false if not.

bool TestRoute(const Route* r)

Have another method TestRouteNeedRefuel that returns true if the route will require refueling, false if not

bool TestRouteNeedRefuel(const Route* r)

Then have a final method, GenerateRefuelRoute that returns a new route with the proper refuel points

Route* TestRoute(const Route* r)
//use this if at all possible
std::unique_ptr<Route> TestRoute(const Route* r)

As far as performance goes, remember to profile before making assumptions. If your colleague is worried about copying Route more than needed (and he may have good reason to, as we don't know how expensive it is or what the target platform is) then clearly performance is an important requirement. I would suggest first implementing as clean an interface as can be done, profiling to find where the bottlenecks REALLY are, and then implementing a few speed hacks where necessary.

Answer 2

I would generally consider returning a pointer from a method in C++ a bad design, and mixing error states and payload data in the return value, too; this is a recipe for unmaintainable code.

Suggested change: Return the fail/success status as int value (or use ternary logic, e. g. boost::tribool), and pass the argument as non-const reference:

/** @returns
    - 1 if a solution has been found. The argument will be updated.
    - 0 if the request has been processed sucessfully, 
      but no (immediate) solution has been found.
      The argument is not modified in this case.
    - -1 if the request failed. The argument is not modified. */
int findSolution(MyClass& argument);

Usage example, leaving out premature optimization to avoid "unnecessary" copies:

MyClass objectToTest(originalUnmutableObject);

switch(findSolution(objectToTest))
{
    case 1:
        //Replace original with updated object, or whatever
        break;
    case 0:
        //Nothing to do (?)
        break;
    case -1:
        //Error handling
        break;
    default:
        //Unexpected return value
        assert(false);
}

An alternative, more sophisticated and reusable approach could be to bundle error state and object into a generic result class; this pattern was inspired by Rust. I leave the implementation of Result to you.

template<typename T>
class Result
{
    public:
        Result() = delete;
        Result(int error);
        Result(const T& data);
        Result(T&& data);

        //Methods
        bool isOk() const;
        bool isError(int error) const;
        int error() const;
        const T& data() const;

    private:
        //Variables
        int m_error = 0;
        T m_data;
};

...

Result<MyClass> findSolution(const MyClass& argument)
{
    int errorCode = 0;

    ...

    if(errorCode != 0)
        return Result(errorCode);
    else if(solutionFound)
        //Error code of result will be 0, Result::isOk() == true
        return Result(update(argument, solution));
    else
        //Error code of result will be 1, Result::isOk() == false
        return Result(1);
}

Answer 3

You should avoid to return raw owning pointer.
You may use smart pointer, and in particularly shared_ptr

Assuming:

bool NoPath(const Route&);
bool NeedRefuel(const Route&);
std::unique_ptr<Route> RouteWithRefuel(const Route&);

you may do:

std::shared_ptr<Route> TestRoute(const std::shared_ptr<Route>& r)
{
    if (!r) {
        throw std::runtime_error("null pointer"); // Or more appropriate exception
    }

    if (NoPath(*r)) {
        // throw NoValidPathException{}; // Alternative
        return nullptr; // No valid Path
    } else if (NeedRefuel(*r)) {
        return RouteWithRefuel(*r); 
    } else {
        return r; // share route: Increase ref counting
    }
}

Answer 4

I feel like this is a case of barking up the wrong tree.

How about a single function to make everyone happy:

Route PlanRoute(Origin, Destination, FuelRange);

The people who don't care about fuel simply pass in a maximum integer. If you can't touch the method signature give this version a new name and make the original name just call this one with a maximum integer for the range.

And I can't understand your colleague--how can reallocating a route be remotely as expensive as generating it in the first place? The only scenario where I can see this making any meaningful difference is if it's being called far more often than the generation (say, checking a fleet of vehicles against the base route) and that could be avoided by modifying the Route object to include the fuel range needed.