Std::pair Is Not OkayPosted on:
Take the time to make a struct or complete class: future devs will thank you.
I came across code equivalent to this abomination recently, written by a long-lost developer in the time before code-reviews. It’s not long, but it’s not great.
The only helpful part here is
ev.mainItemId, which I guess we can assume is some numeric type. The other side of the equality checking is far less helpful.
We could always try to guess what it is from its type but OH NO WAIT it’s of type
auto&. I am not a fan of auto at the best of times, so this is clearly not what I’d want to see.
kvp.second.first? What am I meant to infer from that, if anything?
After scrawling around the header file, it turns out
mStatsCache was of the following fun type:
std::map of a
std::pair, which when iterated, returns a
std::pair<int, std::pair<int, Stat>>: just wonderful.
So, it’s fair to assume from the name that
mStatsCache is a container, but that doesn’t excuse the lack of readability underneath.
A better solution: structs
There’s no point in complaining without offering an alternative, so I’m going to propose a radical, surprising new idea: simply make a damn struct. It’s not hard, it has no overhead compared to the
std::pair (and in fact, may even be cheaper), and increases readability considerably.
Gee willikers, I can read it now! Note I also replaced the
auto& with the actual type. There is the argument to be had that
auto makes the loop look less scary, but if you’re using it to hide a type like
std::pair<some::namespace::MyKeyClass, std::pair<int, some::othernamespace::MyValueClass>> then I’ll say you should either be typedeffing, structing, not bleeding namespaces so much, or sucking it up. I’d rather see clear relevant information immediately, then have it hidden away.
Another solution: classes?
StatData could be written as a class, the obvious advantage of this being default encapsulation of data. If you’re only planning to use the struct in this situation, I’d recommend against this. A struct says to the reader, “I am stupid. I hold data, and do nothing else.” It’s not trying to act like a useful object.
Writing this as a class with all fields publics feels unnecessary. However, if you do get to a point where this struct starts being used elsewhere, with member functions added, I would then say quite the opposite. You would now want a fully fledged class, with the data fields set to private, and data accessed through the necessary getters.
(I promise “wrapping up” wasn’t meant to be a poor encapsulation joke.) But there you have it, why I don’t like
std::pair and what you can do to help avoid it.
Oh and, I asked a colleague what he thought
kvp could stand for. Key-value-pair. Obviously.