std::set doesn't detect duplicate custom objec

Question

I have a map for keeping the team name and players of the team std:pair<std::string, std::vector<std::string> > and a set of pointers to players to keep the players sorted in descending order by wins. And there's the catch - one player can participate in more than one team.

    class Player
{
public:
    int wins;
    std::string name;

    Player() {}
    Player(std::string name) : wins(0), name(name) {}

    bool operator<(const Player* rhs) const
    {
        return this->wins > rhs->wins;
    }

    bool operator()(const Player* lhs, const Player* rhs) const
    {
        return lhs->wins > rhs->wins;
    }
};

int main()
{
    // EXAMPLE
    std::set<Player*> players;
    std::map<std::string, std::vector<Player> > teams;

    teams["TeamA"].push_back(Player("a"));
    teams["TeamB"].push_back(Player("a"));;
    players.insert(&teams["TeamA"].front());
    players.insert(&teams["TeamB"].front());

    std::cout << players.size(); // output 2 

    return 0;
}

As you can see the players in 'TeamA' and 'TeamB' are identical but still in the set are added two pointers and I can't figure it out why.. Is there something I am missing ?

Answer 1

Your set contains pointers, not Player objects. That means pointer comparison is used to determine equivalence.

Naturally the address of the first object in TeamA is different to the address of the first player in TeamB, even if they contain the same data.

If you want to compare by the pointee, use a custom comparator type:

PlayerComp {
  bool operator()(Player* lhs, Player* rhs){
    return *lhs < *rhs;
  }
};

std::set<Player*, PlayerComp> players;

Whereas the player class implements operator< on its own instances, not on pointers:

bool operator<(const Player& rhs) const
{
    return wins < rhs.wins;
}

Answer 2

If my assumptions are correct, this won't work out as you expect!

teams["TeamA"].push_back(Player("a"));
teams["TeamB"].push_back(Player("a"));

I assume here that you want one single player called "a" to be part of two teams. If this player wins within team A, its wins property is incremented, just the same as if it won within team B.

What you actually did, though, is creating two different players, both called "a", and they both appear in the set (well, actually not, see below...), each one maintaining its own wins (first player "a" the wins in team A, second one the wins in team B).

What you would have to do is creating the players in the players' set and add these single instances to the vectors of the various teams:

std::set<Player> players;
std::map<std::string, std::vector<Player*> > teams;
auto entry = players.insert(Player("a"));
teams["TeamA"].push_back(&entry->first);

But this won't work out either: Your comparison is based on the wins only! If you enter two different players (the same will happen, by the way, if you apply StoryTeller's fix!), both start with a win of 0. So both players will compare equal (a < b and b < a both won't apply thus equality), so only one single player will enter the set...

Additionally, std::set has no "auto-update" feature! It requires that its entries remain constant (at least the members of which are used for the comparison!). So if you update your wins on the run of your competitions, the sorting in your set will get lost entirely (apart from that, modifying the keys of a map or set actually is undefined behaviour).

Conclusion: std::set is not an option, at least not the way you want to use it!

Let me propose you another approach:

// yes, a vector...
// we'll need to maintain sorting ourselves...
std::vector<Player*> players;
// need pointers here, too, as pointers to objects
// (those within the teams!) might INVALIDATE on
// vector<Player> resizing!!!

std::map<std::string, std::vector<Player*>> teams;

players.push_back(new Player("a"));
teams["TeamA"].push_back(players.back());
teams["TeamB"].push_back(players.back());

players.push_back(new Player("b"));
teams["TeamA"].push_back(players.back());
teams["TeamC"].push_back(players.back());

Now let us one team win:

for(auto p : teams["TeamA"])
{
    ++p->wins;
}

Fine, we just resort:

std::sort(players.begin(), players.end(), PlayerComp());

Hey, this is just the PlayerComp from StoryTeller's answer... Or we use a lambda instead:

std::sort
(
    players.begin(), players.end(),
    [](Player const* x, Player const* y) { return *x < *y; }
);

Finally, do not forget to delete your player objects again:

for(auto p : players)
{
    delete p;
}

You can skip deletion, if you use smart pointers, e. g. like this:

std::vector<std::unique_ptr<Player>> players;

players.push_back(std::make_unique<Player>("a"));
teams["TeamA"].push_back(players.back().get());

for(auto p : teams["TeamA"])
    ++p->score;
std::sort
(
    players.begin(), players.end(),
    [](std::unique_ptr<Player> const& x, std::unique_ptr<Player> const& y) { return *x < *y }
);

Above assumes the players' vector having unique ownership of the player objects, just as my non-smart-pointer solution does – in consequence, the teams' player pointers all get invalid/dangling, if the players' vector is deleted. Suitable in the specific case, an alternative worth to mention is provided by std::shared_ptr, which avoids the possibility of dangling pointers (price is some object management overhead during creation, assignment and destruction, not, however, while accessing the smart pointer):

std::vector<std::shared_ptr<Player>> players;
std::map<std::string, std::vector<std::shared_ptr<Player>>> teams;

players.push_back(std::make_shared<Player>("a"));
teams["TeamA"].push_back(players.back());


for(auto p : teams["TeamA"])
    ++p->score;
std::sort
(
        players.begin(), players.end(),
        [](std::shared_ptr<Player> const& x, std::shared_ptr<Player> const& y) { return *x < *y; }
);