is it a good style to make a function return the referrence of one of its input parameters?

[steve yee]

the code is like this:

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

but i think this style of function declaration is more flexible, thus
is more reasonal.
i want to know your opinion about it. thanks

 

[Phlip]

Asn_T& Convert(Asn_T& asn)

All the operator<<() methods overloaded for iostreams do that, as a
convenience for chaining. It's just a convenience.
foo(bar).baz().zone().etc().

For a more complex design, consider a function that sometimes returns its
input parameter, and sometimes returns something else.

 

[Alf P. Steinbach]

* steve yee:

the code is like this:

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

but i think this style of function declaration is more flexible, thus
is more reasonal.
i want to know your opinion about it. thanks

Difficult to say without knowing more.

On the one hand, this technique is sometimes used to implement natural
chaining, as in the standard library's iostreams, or in the named
argument idiom*.

On the other hand, it's sometimes used to implement unnatural "concise"
notation that is difficult to decipher.

If had to guess, I'd guess the latter, because it's a reference to the
argument that's returned, not a reference to the object itself.

But on the third hand, returning a reference to the argument is exactly
what you do when implementing an iostream insertion operator, like

std:stream& operator<<( std:stream& stream, T const& obj )
{
return (stream << obj.asString());
}

And one certainly wouldn't want to establish a general principle that
would forbid iostream inserters.



*) <url: http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.18>

 

[Luke Meyers]

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

Just so I understand correctly, the gist of your logic is to convert
the MyData object into an Asn_T, and store that result in the Asn_T
passed in by reference, right?

but i think this style of function declaration is more flexible, thus
is more reasonal.

"Flexible" is a funny word -- everyone has a different idea (or
several) about it. You see the above code as flexible because it gives
you two ways to get the return value. I would argue that it promotes
*inflexibility*, because it promotes inconsistency and an excessively
wide interface. You're providing two ways to do one thing, and
incurring the overhead of both. Being used in two different ways means
you've basically got two interfaces, which makes it harder to change
your code -- *that* is the kind of flexibility you should care about.
When debating high philosophical software design principles, always
remember *why* they're important, otherwise it's easy to fall into
traps like this.

That said, I'd implement it with what I consider the most
straightforward signature:
Asn_T MyData::Convert() const;

I made it const, because I don't think a member function called
Convert() would be reasonable if it modified *this. I prefer to simply
return by value, because it provides less opportunities to screw up
object lifetimes. I left out the parameter because it's unnecessary,
unless there's something you're not telling us about what this function
needs to do.

Luke

 

[steve yee]

Just so I understand correctly, the gist of your logic is to convert

the MyData object into an Asn_T, and store that result in the Asn_T
passed in by reference, right?
Yes.

That said, I'd implement it with what I consider the most
straightforward signature:
Asn_T MyData::Convert() const;

But this will cause overhead of temp object. You have to implement
MyData::Convert() this way:
Asn_T MyData::Convert()
{
Asn_T tempObject;
// convertin codes ...
...
return tempObject;
}
The construct/destruct/copy of tempObject will cause low effeceicy, do
you think so?

Being used in two different ways means
you've basically got two interfaces, which makes it harder to change
your code -- *that* is the kind of flexibility you should care about.

But such a simple function which does exactly only one thing
--converting itself to another type, is unlikely to change the type of
return value, nor is it neccessary. That is, the flexibility of
changing the code does not exist at all, so consideration should be
focused on the flexibility of the way to use the code.

BTW, how do you think of those ansi C functions which also return its
input parameters? such as: strcpy, memcpy, memmove, ...

 

[Marcus Kwok]

But this will cause overhead of temp object. You have to implement
MyData::Convert() this way:
Asn_T MyData::Convert()
{
Asn_T tempObject;
// convertin codes ...
...
return tempObject;
}
The construct/destruct/copy of tempObject will cause low effeceicy, do
you think so?

Not necessarily, look up RVO (return value optimization) and NRVO (named
RVO).

 

[Noah Roberts]

the code is like this:

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

but i think this style of function declaration is more flexible, thus
is more reasonal.
i want to know your opinion about it. thanks

Chaining is one reason to do the above and another is to allow you to
portably return values without the cost of many copy operations.

void fun(const Ast_T & arg);

MyData d;
Ast_T t;

fun(d.Convert(t));

You can't do that if the function returns void. It is really what the
purpose of the argument is. If the argument is being passed in to
avoid temporaries then returning it as well is quite appropriate. For
instance, since you don't know how std::string works this is a good way
to return one - accept it as a parameter and return it too.

If the return value is seldom used however they have a point. You
don't want to encourage chaining except in very narrow situations. It
quite quickly reaches into the realm of unreadability.

 

[Jonathan Mcdougall]

the code is like this:

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

Several people have pointed that returning an Asn_T would allow
function chaining. However, the chaining idiom is usually seen using
the object on which the function was called

my_object.set_value(a).set_value(b).set_value(c);

not on one of its parameters.

IMO, either return an object or pass one by reference, but don't do
both.

but i think this style of function declaration is more flexible, thus
is more reasonal.

By "reasonal", do you mean "reasonable"? If so, something flexible is
not necessarily reasonable. There is a compromise to make between
flexibility (genericity) and usability.

If I understand your code correctly, Convert() converts the object to
an Asn_T. In this case, it makes more sense to create a new Asn_T
object and to return it by value. If I don't have an Asn_T prior to
calling Convert(), I must create one before.

void f(MyData& d)
{
Asn_T a;
d.Convert(a);
}

I read this as "I am creating an Asn_T and then using it to convert a
MyData". This implies default construction of an Asn_T (which could be
expensive) and is less clear semantically than

void f(MyData& d)
{
Asn_T a = d.Convert();
}

which I read as "I am creating an Asn_T specifically to convert a
MyData". Convert() should therefore look like

Asn_T MyData::Convert()
{
Asn_T a;
// ...

return a;
}


Jonathan

 

[Noah Roberts]

the code is like this:

class Asn_T
{
public:
DataType_T m_data;
};

class MyData
{
MyDataType m_data;
public:
Asn_T& Convert(Asn_T& asn)
{
// coverting codes goes here

return asn;
}
};

in a code review, some members of our team argued strongly that the
function MyData::Convert() should not return Asn_T&, instead, it should
be declared as returning void,because the return value is seldom used,
and it's enough to use the input parameter.

but i think this style of function declaration is more flexible, thus
is more reasonal.
i want to know your opinion about it. thanks

Chaining is one reason to do the above and another is to allow you to
portably return values without the cost of many copy operations.

void fun(const Ast_T & arg);

MyData d;
Ast_T t;

fun(d.Convert(t));

You can't do that if the function returns void. It is really what the
purpose of the argument is. If the argument is being passed in to
avoid temporaries then returning it as well is quite appropriate. For
instance, since you don't know how std::string works this is a good way
to return one - accept it as a parameter and return it too.

If the return value is seldom used however they have a point. You
don't want to encourage chaining except in very narrow situations. It
quite quickly reaches into the realm of unreadability.

 

[Luke Meyers]

The construct/destruct/copy of tempObject will cause low effeceicy, do
you think so?

No, due to return value optimization performed by essentially all
modern C++ compilers. Really, though, that's besides the point.
Premature optimization is a bad idea in general -- that's not to say
you shouldn't try to write efficient code, but there is a line and this
is on the wrong side of it. Save this sort of micromanagement for
cases where you've discovered and measured a specific performance
problem associated with the code in question. Too many attempted
optimizations turn out to be useless, confusing, or actually less
efficient. Trust the compiler to do the right thing most of the time.
If you're concerned about performance, use a profiler.

But such a simple function which does exactly only one thing
--converting itself to another type, is unlikely to change the type of
return value, nor is it neccessary. That is, the flexibility of
changing the code does not exist at all, so consideration should be
focused on the flexibility of the way to use the code.

Why? Remember how I constrasted the kind of flexibility you're talking
about (multiple interfaces) with the kind of flexibility that's
important and helpful (making code easy to change/refactor). Whether
there is or is not a meaningful opportunity here to introduce the
latter, beneficial form of flexibility has nothing to do with implying
that the kind of flexibility you refer to is desirable in this or any
case. Excessively wide interfaces are problematic, in part because
they promote inconsistent use. Inconsistency makes code harder to
maintain because expectations tend to be incorrect -- read up on the
principle of least surprise.

BTW, how do you think of those ansi C functions which also return its
input parameters? such as: strcpy, memcpy, memmove, ...

Those interfaces were determined a long time ago, and a lot of software
engineering thinking has occurred in the mean time. I'm not saying
they're bad -- I don't use them frequently enough to have a strong
opinion on whether this practice is a good idea in those cases. I
wouldn't imitate them as a matter of course, though, by any means.

Luke

 

[Noah Roberts]

No, due to return value optimization performed by essentially all
modern C++ compilers. Really, though, that's besides the point.
Premature optimization is a bad idea in general -- that's not to say
you shouldn't try to write efficient code, but there is a line and this
is on the wrong side of it. Save this sort of micromanagement for
cases where you've discovered and measured a specific performance
problem associated with the code in question. Too many attempted
optimizations turn out to be useless, confusing, or actually less
efficient. Trust the compiler to do the right thing most of the time.
If you're concerned about performance, use a profiler.

There is a flip side in this case. If you do find that this is a
performance issue then you are going to be so tied to the interface
that getting out will be difficult at best. Interface decisions should
take performance in mind at least a little bit as it will be hard to
change them later.