Std::pair Is Not Okay

Posted 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.

I.
Erm.

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. kvp? Really?

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.
And 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:

An 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?

Now, 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.

Wrapping up

(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.



c++, oop, std