Is it a good design to return value by parameter?

Question

bool is_something_ok(int param,SomeStruct* p)
{
    bool is_ok = false;

    // check if is_ok

    if(is_ok)
       // set p to some valid value
    else
       // set p to NULL
    return is_ok;
}

this function return true and set p to a valid value if "something is ok" otherwise return false and set p to NULL

Is that a good or bad design? personally, i feel uncomfortable when i use it. If there is no document and comment, i really don know how to use it.

BTW:Is there some authoritative book/article about API design?

Answer 1

I think to return NULL if something is not OK and valid SomeStruct if OK is better in this case

SomeStruct* is_something_ok(int param);

In this case except of checking boolean value you should check is it NULL, and if not use it.

But there is cases when you have to return value by parameter. It depends from the number of return values, and the context function can be used.

Answer 2

You can do the following:

bool is_something_ok(int param,SomeStruct* p);

or

// return NULL if the operation failed.
Somestruct* do_work(int param);

It is hard to say if a design/API is good or bad, nothing is black or white ... (grey?!?!?)

You must choose the API/Standard which will be easier for you to code with. And be coherent, if you choose the first method type, do it for the rest of your project.

Dont also forget to document your code, so It will be more easier to understand how to use your API.

Answer 3

I tend to do this. The alternative in your example is to either encode two things into one return value (For example using NULL as a special value) or to return a structure.

Encoding two things can sometimes be impossible, and is a little error prone. Returning a structure is a lot of extra work and clutter. So I tend to do what you've done. I tend to assume that "raw" pointers and references in a parameter list are for returning values in, and they would be "const" if they were only for passing data in.

But to be honest I forget that rule as often as I remember it so perhaps it's not a very good one.

There is a class "optional" in the boost library which might fit your needs but I never really liked it myself for perhaps no very good reason.

Answer 4

Depends on how you want to handle 'errors'.

For example, take the standard function atoi. It converts a string to an integer, but if the string does not contain a number, what should it return? In this case, the C/C++ runtime will set the errno global variable. An alternative would be to throw an exception.

Personally, I don't really like both of these alternatives. Therefore, if I generally use the following rules:

• Is there a value in the range of possible return values that can be used to indicate an error, and do I only have 1 possible 'kind of' error? In those cases, I use the return value to indicate the error. E.g. a function like FindEmployee could simply return NULL if the employee is not found.
• If all possible values could be returned by the function (as in the atoi example), use an output argument for the return value, and let the function return a boolean. If you have more than 1 possible error case, return an enum that indicates the kind of error (or success) that occurred.

Answer 5

Since you have tagged the question as C++ and not C, I would suggest you to:

• return the value directly
• if you have more than one value, use output parameters
• use non-const references as output parameter where possible (instead of pointers), and use const-references for the input parameters.
• if something went wrong, raise a exception instead of returning false or -1.

But that are just some general hints. The best way to go always depends on the specific problem...

Answer 6

I would say it depends. How expensive is your type to copy construct? Can you write your function RVO-friendly? At least until we have C++0x with rvalue references, my recommendation would be not to return "expensive" types (such as std::vector<std::string>), but rather pass them as references, e.g. use:

void split(const std::string &txt, char sep, std::vector<std::string> &out);

rather than:

std::vector<std::string> split(const std::string &txt, char sep);

Depending on how you write your function it's possible that RVO will kick in but in my experience it's not something you can generally rely on.

Answer 7

You should look into boost::optional when you want to return a value only on success.

Answer 8

I would suggest to return the the Result-Type directly like so:

SomeStruct doSomething(int param) {...}

and throw an exception in cases that the function can't handle (tux21b already mentioned this way). Alternatively you can return two types with std::pair without throwing an exception like so:

pair<SomeStruct, bool> doSomething(int param) {...}

And third i like to declare output parameters as pointers instead of references (like you mentioned) because in the calling code i see the difference of input and output parameters. Given the function:

void doSomething(const Somestruct& in, Somestruct* out) {...}

Then in the calling code it is visible (without looking at the function declaration) what's the input and what's the output parameter (if i consecently apply this concept).

SomeStruct a;
SomeStruct b;
doSomething(a, &b); // you see a is input, b is output