Returning result via Output-parameter, c++ coding standard

Question

I have a member function in my class like this:

int MyClass::m_Func(int& val);

in which, I do some operation and put the result in val. And depending upon the result of the operation, I returned different values from the function. Like, if it is successful, I return 0 or other values if any error occurs.

One of my friend told me that it is not a good practice to pass a reference of a variable to a member function. Is it true? If yes, why so?

Solution 1

The main reason why this practice is somewhat suspicious, and why I'd generally recommend against it, is that users, when they call your function,

int val;
obj.my_Func(val);

may not be aware of the fact that the value of val is in fact modified. This may be more or less obvious when the function has only one parameter and no return value other than an error code, but as soon as you have a slightly more complex function call, e.g.

int x,y,rad,len;
obj.calculate_coord(x,y,rad,len);

the intention may be to calculate rad and len from x and y, or vice versa, or something entirely different. Since the argument names are chosen by the user, and their order or types do not give any hint on which ones serve as input and which ones as output parameters, this can cause misunderstandings, hence make the code harder to maintain and give rise to bugs.

On the other hand, it is quite straight-forward to use C++ syntax to properly return not just the value or the error code, but a combination of one or more values and an error code.

In the simplest case you may use a function like this (using C++11 syntax):

std::tuple<int,int,int> my_Func(int input)
{
  /* calculate.. */

  return { error_code , val1 , val2 };
}

to return two values and an error code. There is no question that input is a pure input parameter (passed by value) and the return value is complex and includes everything you might want to return.

Contrary to what some programmers may suggest, this does not usually cause a lot of unnecessary copying, even when the return value is much larger (because of return value optimization, and in C++11 because of move semantics).

There may be exceptions, especially in pre-C++11, i.e. special cases where move semantics can't be used or for some reason return value optimization doesn't apply. There may also be cases where code profiling reveals that a) the function is used extremely heavily, and b) for whatever reason, passing the output through a reference is faster. In these cases, passing an output parameter by reference may be the right technique to use.

---

Of course, what I said above applies only to the use of references as output parameters, i.e. as an indirect way of returning values to the user, in particular when the fact that the function modifies some of its parameters isn't made obvious by its name.

There can be many other, and valid, reasons for passing by reference, esp. by const-reference.

Solution 2

I guess, that your friend did not actually complain about passing a reference to a variable to a method rather than about the error reporting technique you've chosen (actually, people usually are encouraged to pass parameters by reference, it's a lot safer than passing by pointer). In C++, one usually uses exceptions to check for errors; your code is a little C-style. So instead of:

int res;
myInstance->m_Func(res);
if (res != 0)
    // do sth

I'd rather write:

MyClass::m_Func()
{
    if (some_condition)
        throw std::exception("Error!");
}

// (...)

try
{
    myInstance->m_Func();
}
catch (...)
{
    // do sth
}

Solution 3

Disclaimer: Opinions will differ wastly on this one, since it is a matter of style, convention, etc.

What you have there is a so-called "out-parameter", i.e. your function does not pass its result through the returned value, but through the parameter. This is mainly because the returned value is occupied by the error code (i.e. the success indicator).

In many languages like C, this is a common usage of functions and error handling. In some applications of C++ where using exceptions is not possible for some reason, this style may be adopted as well. In "pure" C++ however, you have exceptions for the error handling. It is common that any parameter to a function is an input parameter, and any value prodced by the function is passed out via the returned value. If there is more than one value to return, the values are bundled in a std::pair, std::tuple or some struct.

Using exceptions might be too harsh in your case, maybe because non-success is not really an exceptional case but a common and expected outcome of the function. In that case, return a pair of values - one success indicator and one result value. Bundling them together might look odd at first but is a viable solution, since you have to evaluate them both anyways. But remember, in most cases it will be cleaner to throw an exception if the function cannot execute its task, i.e. calculate a useful return value.

Comments

[Robert S. Barnes]

Just my opinion, but exceptions should only be used for things that should never happen, like violating basic assumptions or other rare events like out of memory. It shouldn't be used for regular error handling.

[Arne Mertz]

@RobertS.Barnes that depends on the definition of "error". If the function is called in a condition where it cannot execute its task in a meaningful way, it is an error that should be handled via exceptions. It's the caller's responsibility to establish the preconditions for the function.

[Paul Rademacher]

RVO will not be invoked for the values inside tuples, so you won't get that optimization from compiler. Also, if you pass in, e.g., a std::vector variable to make_tuple, move semantics won't be invoked unless you explicitly call std::move. So it will do unnecessary copying.