CODEWITHSUNDEEP
c++argumentsreturn-type
Reasurria
Reasurria

Reputation: 1858

C++ returning a string reference and passing a string reference in the same function

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

Answers (4)

Tony Delroy
Tony Delroy

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

DamnnnSure
DamnnnSure

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

Angew is no longer proud of SO
Angew is no longer proud of SO

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

Joseph Mansfield
Joseph Mansfield

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

Related Questions