Reputation: 1858
I have been arguing with my superior about this function:
const std::string &GetCurrentDataSourceName(std::string & sName)
{
sName = GetAnotherComponent().GetName();
return sName;
}
Is there any reason for the function return type and the parameter type to both be included? The purpose of the function is to return 1 value.
His motivation and use case is that one can do this:
std::string sName = "";
SetSomeValue(GetCurrentDataSourceName(sName));
I would think it is better to leave out the parameter like so:
const std::string &GetCurrentDataSourceName()
{
return GetAnotherComponent().GetName();
}
But he has me doubting in my coding ability.
Edits: The value returned must be const. The code has also been updated to show where the return value comes from. I comes from another component inside the same class;
Upvotes: 2
Views: 3204
Reputation: 106254
Your proposed code...
const std::string &GetCurrentDataSourceName()
{
return somevalue;
}
...is worrying, as it returns a reference to a variable which means you have to have some deep insights and assurances about how the lifetime and potential further changes to / invalidation of somevalue
relates to potential client usage.
That's fine if you happen to be returning a reference to say a static const std::string
storing a "component" name (as implied by your edited question), but it's normally not something you want as very often return by reference is accidental and the reference is to a (called-)function-local variable that goes out of scope as the function returns, so anyone seeing the return type above will immediately get nervous about the function and at very least want to carefully check the implementation or ensure the documentation gives related assurances.
Generally it's better to code...
std::string GetCurrentDataSourceName() // "const" here if a member function
{
return ...somevalue..;
}
...which - if there's any need to set a local variable too - can then be used like so:
SetSomeValue(sName = GetCurrentDataSourceName());
It's also more convenient as the caller doesn't necessarily need a named local variable if they just want to e.g. pass the data source name to another function, use it in an expression (e.g. adding prefixed/quoted, escaping it for use in a URL), or stream it somewhere.
In this case, there's ostensibly no reason to both accept the variable to be set as a by-reference parameter and return the new value.
Upvotes: 2
Reputation: 21
I don't think the 'convenient to concat function calls in one line' is a valid reason to do so.
Better return by value rather by const reference. Especially when you can use c++11.
std::string GetCurrentDataSourceName(std::string sName)
{
//do some work with sName..
return sName;
}
since you will modify sName, you're gonna make copy anyway, with move semantic, you can save the copy overhead with function call like this:
sName = GetCurrentDataSourceName(std::move(sName)); //no copy required
Upvotes: 1
Reputation: 171197
The original function looks like an ugly hack. It certainly invokes a "WTF" from anyone reading the code, and as such it should probably be avoided.
Note that your replacement relies on somevalue
being a global object or similar. If that's not the case (i.e. if somevalue
is computed inside the function), you'd be returning a dangling reference - buggy code.
I'd say the cleanest way would be to get rid of one-liner-enabling hacks, rely on move semantics and/or [N]RVO to do their job, and just return by value:
std::string GetCurrentDataSourceName()
{
return somevalue;
}
Upvotes: 7
Reputation: 110778
It seems like the whole point of the function is just to get some string value, in which case I would just write it like this:
std::string GetCurrentDataSourceName() {
return somevalue;
}
Unless you have a good reason to return a const
reference to somevalue
, just return by value. If somevalue
is being computed in the function and you've just omitted, then definitely don't return a reference.
I see no good reason to have the output parameter. Your superior's example can just be written like this:
std::string sName = GetCurrentDataSourceName();
SetSomeValue(sName);
I also find this much easier to read.
Upvotes: 5