Problem using a compare function inside of a class for sort()

Question

If you take a look at my function CardCompare inside of the class... It doesn't work! But, if I instead use the function where it is commented out in Hand.cpp, it works perfectly fine. Why is this?

Also, I am wondering if keeping the CardCompare function in my hand class makes less sense than keeping it in the Card class, if that is possible.

Hand.h

#ifndef HAND_H
#define HAND_H

#include <vector>
#include "Card.h"

class Hand {
    private:
        std::vector<Card> hand;
        int total;
        void CalculateTotal();
        bool CardCompare (Card i, Card j) {return ( i.RankInt() < j.RankInt() ); }//Does not work! :O
    public:
        Hand() {
            total = 0;
        }
        std::vector<Card> GetHand() const{ return hand;};
        void PrintHand();
        void AddToHand(Card c);

};



#endif

Hand.cpp

#include "Hand.h"
#include <iostream>
#include <algorithm>

void Hand::CalculateTotal() {
    for (int i = 0; i < hand.size(); i++) {
            std::cout << hand[i].ToString() << std::endl;
    }
}

void Hand::PrintHand() {
    for (int i = 0; i < hand.size(); i++) {
        std::cout << hand[i].ToString() << std::endl;
    }
    std::cout << std::endl;
}
/* If I place this right here, it works perfect.
bool CardCompare (Card i, Card j) {return ( i.RankInt() < j.RankInt() ); }
*/
void Hand::AddToHand(Card c) {
    hand.push_back(c);
    std::sort(hand.begin(),hand.end(),CardCompare);
}

int main() {
    Hand h;
    h.PrintHand();
    h.AddToHand(Card (2, ( Card::Suit )2 ) );
    h.PrintHand();
    h.AddToHand(Card (3, ( Card::Suit )3 ) );
    h.PrintHand();
    h.PrintHand();
    h.AddToHand(Card (1, ( Card::Suit )2 ) );
    h.PrintHand();
    h.AddToHand(Card (13, ( Card::Suit )3 ) );
    h.PrintHand();

    std::cout<< std::endl << std::endl;

    std::cout << h.GetHand()[0].ToString();
}

Card.h

#ifndef CARD_H
#define CARD_H

#include <string>

class Card {
public:
    enum Suit {
        SUIT_HEART,
        SUIT_DIAMOND,
        SUIT_CLUB,
        SUIT_SPADE
    };
    Card(int r = 1, Suit s = SUIT_HEART) : rank(r), suit(s)
    {}
    int GetRank() const { return rank; };
    Suit GetSuit() const { return suit; };
    std::string ToString() const;
    std::string SuitString() const;
    std::string RankString() const;
    int RankInt() const;


private:
    int rank;
    Suit suit;
    static const char * ranknames[];
    static const char * suitnames[];
    static const int     rankints[];
};
#endif

Card.cpp

#include <iostream>
#include "Card.h"
//#include <vector> //gtfo

const char * Card::ranknames[] = { "A", "2", "3", "4", "5", "6", "7", "8", "9", "10", "J", "Q", "K" };
const char * Card::suitnames[] = { "Hearts", "Diamonds", "Clubs", "Spaces" };
const int    Card::rankints[]  = {11, 2, 3, 4, 5, 6, 7, 8, 9, 10 ,10 ,10, 10 };

std::string Card::ToString() const {
    std::string s = RankString();
    s.append(" of ");
    s.append(SuitString());
    return s;
}

std::string Card::SuitString() const {
    return suitnames[suit];
}

std::string Card::RankString() const {
    return ranknames[rank-1];
}

int Card::RankInt() const {
    return rankints[rank-1];
}
 /*
int main() {

    std::vector<Card> Deck;

    for (int i = 0; i < 10 ; i++) {
        Deck.push_back(Card(i+1,(Card::Suit)((i+1)%4)));
        std::cout << Deck[i].ToString() << std::endl;
    }

    std::cout << std::endl << std::endl;
    std::random_shuffle( Deck.begin(), Deck.end() );

    for (int i = 0; i < 10 ; i++) {
            std::cout << Deck[i].ToString() << std::endl;
    }
}*/

Personal preference: Rather than casting numbers to Card::Suits, you should have static members of Card::Suit called Spades, Hearts, Diamonds, and Clubs. — Chris Lutz
– Chris Lutz, Commented Nov 26, 2010 at 21:13
@Chris Lutz Could you explain in more detail? I'm still a nub and am not sure what you mean. — Colton
– Colton, Commented Nov 26, 2010 at 21:30
@Colton - class Card { public: class Suit { static const Suit Spades, Hearts, Diamonds, Clubs; ... }; ... }; const Card::Suit Card::Suit::Spades = 0, Card::Suit::Hearts = 1, Card::Suit::Clubs = 2, Card::Suit::Diamonds = 3; Then you can add methods to the Card::Suit class, like Card::Suit::isblack() and such. Users of your class, instead of having to know that (Card::Suit)0 is for spades, can just say Card::Suit::Spades. — Chris Lutz
– Chris Lutz, Commented Nov 26, 2010 at 21:38

Yakov Galka · Accepted Answer · 2010-11-26 20:53:54Z

9

You are trying to pass a pointer to a member function, so sort can't use it because it doesn't have this pointer. In your case you can just change the function to be static:

static bool CardCompare (Card i, Card j) {return ( i.RankInt() < j.RankInt() ); }

If you do need it be a non-static member function in the future, bind it with boost::bind or std::bind (for C++0x compiler):

std::sort(hand.begin(),hand.end(),bind(&Hand::CardCompare, this, _1, _2));

answered Nov 26, 2010 at 20:53

Yakov Galka

72.8k16 gold badges148 silver badges226 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Colton Over a year ago

Sorry, a bit confused. So basically the sort() takes a pointer to a function, but since I didn't have it static, it could only be associated to an instantiation of an object? And since it is associated to a instance it would need the "this" pointer? Am I on the right track here?

Yakov Galka Over a year ago

@Colton: sort takes anything that it can call like this: compare(a,b). It can be a function pointer or an object with operator() overloaded. It can't call a member function by member(a,b), thus it doesn't work.

irrelephant · Accepted Answer · 2010-11-26 21:02:27Z

4

CardCompare() cannot be a member function if it is to be used in sort(). You can just overload operator< in the Card class to compare cards.

In card class, something like:

bool operator<(const Card& other) const {
    return RankInt() < other.RankInt();
}

edited Nov 26, 2010 at 21:02

answered Nov 26, 2010 at 20:55

irrelephant

4,1112 gold badges28 silver badges41 bronze badges

2 Comments

Colton Over a year ago

With this solution, how would you call sort in my case? sort (hand.begin(), hand.end(), Card() ); ?

Yakov Galka Over a year ago

@Colton: sort(hand.begin(), hand.end()); By default sort compares with <.

Stack Overflow is garbage · Accepted Answer · 2010-11-26 21:00:28Z

1

The canonical solution is to overload operator() instead. This turns your class into a functor, and then it'll work out of the box with the standard library algorithms.

Just change

bool CardCompare (Card i, Card j) {return ( i.RankInt() < j.RankInt() ); }

to

bool operator()(Card i, Card j) {return ( i.RankInt() < j.RankInt() ); }

and then you can call sort like this:

std::sort(hand.begin(),hand.end(), Hand());

Normally, you'd put the comparison operator in a separate class though

answered Nov 26, 2010 at 21:00

Stack Overflow is garbage

249k53 gold badges356 silver badges558 bronze badges

5 Comments

Colton Over a year ago

So basicall by overloading the operator() function it allows my Hand class to be used with most of the STL algorithms as a container without having to make my own compare() functions (and whatever else I might have to do, still new to algorithms) And if I should put the comparison operator in a seperate class, then what class?

Yakov Galka Over a year ago

@Colton, @jalf I think that adopting this code exactly as suggested is not a good idea. jalf, you're creating Hand() (with a vector inside) each time want to sort. Either call it with std::sort(..., *this); or if you want to write operator() in Hand for comparison, it's better to write operator < instead.

Colton Over a year ago

@ybungalobill Oh okay, for example the paramater could determine whether its sorted in ascending or descending order right? Or whether to sort based on SUIT or Rank, etc. Cool. Last thing, this struct would be kept in the Hand file?

Yakov Galka Over a year ago

@Colton: Oops, I've deleted my second comment because it almost repeated jalf's answer... Yes you can put it in Hand file, even as a private struct inside Hand class.

Stack Overflow is garbage Over a year ago

@Colton: it could, but it doesn't have to. I usually keep such comparer functors as entirely separate structs. But put it where you feel it makes sense.

Collectives™ on Stack Overflow

Problem using a compare function inside of a class for sort()

3 Answers 3

2 Comments

2 Comments

5 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

2 Comments

2 Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related