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?

  • 6
    Possible duplicate of Better to have 2 methods with clear meaning, or just 1 dual use method?
    – gnat
    Oct 25 '17 at 12:49
  • 3
    And how is the caller supposed to distinguish a total failure from a trivial success? Oct 25 '17 at 12:49
  • @KilianFoth By checking a value of "LastErrorCode"...
    – Greg82
    Oct 25 '17 at 12:51
  • 2
    Don't know all the details, but maybe what you should do is separate out TestRoute into two methods - one that checks the route (tests if the destination is reachable given a certain amount of fuel), and another that recomputes a new route? Presumably, this recomputation is more expensive then what Planner does? And maybe you could hide all that logic behind something, so that the client code works with a uniform interface. Oct 25 '17 at 13:12
  • 4
    Rather than splitting/adding more methods why not go the other way and make the original method return a route that already includes tank stops if those are necessary to reach the destination? I don't see why it should be the callers responsibility to make sure the route is feasible. Oct 25 '17 at 13:39

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.

  • Make TestRoute private and call it by Planner was my first design, but we had to change it due to existing code...
    – Greg82
    Oct 25 '17 at 13:15
  • 1
    @Greg82 Ah, that's disappointing. Legacy code... this is why we can't have nice things. If there are any other constraints due to legacy code, you may want to update your question to describe what they are.
    – Ryan
    Oct 25 '17 at 13:29

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);

    case 1:
        //Replace original with updated object, or whatever
    case 0:
        //Nothing to do (?)
    case -1:
        //Error handling
        //Unexpected return value

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
        Result() = delete;
        Result(int error);
        Result(const T& data);
        Result(T&& data);

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

        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));
        //Error code of result will be 1, Result::isOk() == false
        return Result(1);
  • I think you mean that the argument is updated if the method returns 0, and not updated if it returns 1? But I cannot modify the original object...
    – Greg82
    Oct 25 '17 at 13:10
  • @Greg82 Read the code comments; it's meant vice versa by me; however it's up to you to define the meaning of the return values of your method. If you can't modify the object then make a copy of it before passing it to the method.
    – Murphy
    Oct 25 '17 at 13:16
  • But the debate was precisely to avoid a useless copy when a solution is found immediately :)
    – Greg82
    Oct 25 '17 at 13:18
  • That seems contradictionary to me; you need to pass a mutable object if you want it to be modified by the method, no matter if you use pointers or references.
    – Murphy
    Oct 25 '17 at 13:25
  • 1
    I expanded my example, and I suggest you don't think about premature optimizations (avoiding copies; they are probably cheap in comparison with your route calculations) in favour of read- and maintainable code. Good design first, necessary optimization later.
    – Murphy
    Oct 25 '17 at 13:47

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


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

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.

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.