CodeSOD: An Exert Operation
The Standard Template Library for C++ is... interesting. A generic set of data structures and algorithms was a pretty potent idea. In practice, early implementations left a lot to be desired. Because the STL is a core part of C++ at this point, and widely used, it also means that it's slow to change, and each change needs to go through a long approval process.
Which is why the STL didn't have a std::map::containsfunction until the C++20 standard. There were other options. For example, one could usestd::map::count, to count how many times a key appear. Or you could use std::map::findto search for a key. One argument against adding astd::map::containsfunction is thatstd::map::count basically does the same job and has the same performance.
None of this stopped people from adding their own. Which brings us to Gaetan's submission. Absent a std::map::contains method, someone wrote a whole slew of fieldExists methods, where field is one of many possible keys they might expect in the map.
bool DataManager::thingyExists (string name){ THINGY* l_pTHINGY = (*m_pTHINGY)[name]; if(l_pTHINGY == NULL) { m_pTHINGY->erase(name); return false; } else { return true; } return false;}I've head of upsert operations- an update and insert as the same operation, but this is the first exert- an existence check and an insert in the same operation.
"thingy" here is anonymization. The DataManager contained several of these methods, which did the same thing, but checked a different member variable. Other classes, similar to DataManager had their own implementations. In truth, the original developer did a lot of "it's a class, but everything inside of it is stored in a map, that's more flexible!"
In any case, this code starts by using the [] accessor on a member variable m_pTHINGY. This operator returns a reference to what's stored at that key, or if the key doesn't exist inserts a default-constructed instance of whatever the map contains.
What the map contains, in this case, is a pointer to a THINGY, so the default construction of a pointer would be null- and that's what they check. If the value is null, then we erase the key we just inserted and return false. Otherwise, we return true. Otherotherwise, we return false.
As a fun bonus, if someone intentionally stored a null in the map, this will think the key doesn't exist and as a side effect, remove it.
Gaetan writes:
What bugs me most is the final, useless return.
I'll be honest, what bugs me most is the Hungarian notation on local variables. But I'm long established as a Hungarian notation hater.
This code at least works, which compared to some bad C++, puts it on a pretty high level of quality. And it even has some upshots, according to Gaetan:
[Advertisement] Picking up NuGet is easy. Getting good at it takes time. Download our guide to learn the best practice of NuGet for the Enterprise.On the bright side: I have obtained easy performance boosts by performing that kind of cleanup lately in that particular codebase.