Does anyone find this code being very bad?
Can you rate this from 0 to 10. I am very interested what is your opinion on returning a reference that is nullptr.
const std::string &SoundHandler::getPlayingFromPlaylist(const std::string& playlistName)
{
//Check to see if the playlist exists
auto iter = playlists.find(playlistName);
if(iter == playlists.end())
return nullptr;
return playlists[playlistName].currentlyPlaying;
}
0.
Your compiler should be warning you that you're returning a reference to a temporary. If it isn't, get a better one, and make warnings errors.
I am very interested what is your opinion on returning a reference that is nullptr.
That's not what that code is doing, per se; that code is creating a std::string temporary out of the null pointer (via the constructor that takes a const char *) and returning that.
how bad is my teammate?
I wouldn't be too quick to judge, it would be easy for this code to come into being via a refactor that changed to std::string from using C-style const char* strings, or any other number of incremental changes. The real problem is that you're not compiling with the tools to catch these issues (or you are, and you're letting people check in with compile errors or without compiling). Either way the larger failure is on the process, and calling him a bad programmer isn't (necessarily) fair.
There are other flaws with the code as well (you search the map twice, once in the find call, and then redundantly again in the bottom-most return call where you can instead make use of the iterator).
I never meant it as an insult, i just couldn't force him to change it 
. I expect references to be valid, if he cannot ensure it, return a pointer.
After a laugh he did a quick fix 
, thx on the opinions.
EDIT:: The compiler gave a warning, he though its okay.
In situations where it can optionally return nothing, I'd prefer something list std::optional / boost::optional.
What I actually do in these situations, is return a const reference to a static empty version and log a warning/error. Not ideal, but better than a lying reference. However, I've been deciding more and more that asserts are my friend.
I thought it would be something like that... Except isn't nullptr supposed to solve this problem (compared to NULL/0) where null pointers weren't typesafe and thus caused people to unknowingly call the wrong function?that code is creating a std::string temporary out of the null pointer (via the constructor that takes a const char *) and returning that.
Your compiler should be warning you that you're returning a reference to a temporary. If it isn't, get a better one, and make warnings errors.
We've probably been over this before, but why isn't this covered by the most important const thingymajig?
Returning nullptr there will invoke undefined behavior as nullptr does not point at an array of at least traits::length(s)+1 elements of CharT, which is a requirement of this overload.
There was probably good intent behind this (so I'm still giving 2 points rather than 0), assuming that std::string would make something meaningful (empty string, or "the null string") out of it, but it's really just undefined behavior.
(The reference to temporary thing isn't really much of an issue since it's const, so its lifetime is extended to the lifetime of the reference.)
EDIT:: The compiler gave a warning, he though its okay.
That is why I love -werror or /WX. Reduces the chances of idiot programmers to ignore the warnings.
EDIT:: The compiler gave a warning, he though its okay.
That is why I love -werror or /WX. Reduces the chances of idiot programmers to ignore the warnings.
It's really easy to ignore warnings, or just miss them if you don't seem them. Having warnings as errors can seem like a pain, especially if turned on more than halfway through a project, but it's totally worth doing. (And it's best done from the beginning)
