4

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?

  • 4
    Possible duplicate of Better to have 2 methods with clear meaning, or just 1 dual use method? – gnat Oct 25 '17 at 12:49
  • 2
    And how is the caller supposed to distinguish a total failure from a trivial success? – Kilian Foth Oct 25 '17 at 12:49
  • @KilianFoth By checking a value of "LastErrorCode"... – Greg82 Oct 25 '17 at 12:51
  • @gnat How is it related to my question? – Greg82 Oct 25 '17 at 12:52
  • 2
    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. – Roman Reiner Oct 25 '17 at 13:39
8

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. As they say, Pre-optimization is the root all evil :)

  • 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
  • @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
5

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);
}
  • 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
1

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
    }
}

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.