6
\$\begingroup\$

I have written a working implementation of a Poker card that I want to be comparable. I feel that my solution is somewhat bloated in C++, and want to ask if I should have done something different to make it more concise.

#ifndef POKER_CARD_H
#define POKER_CARD_H

enum Value { Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace};
enum Color {Hearts, Spades, Dimonds, Clubs};

namespace {
    int as_int(Value value){
        switch(value){
            case Value::Two     : return 2;
            case Value::Three   : return 3;
            case Value::Four    : return 4;
            case Value::Five    : return 5;
            case Value::Six     : return 6;
            case Value::Seven   : return 7;
            case Value::Eight   : return 8;
            case Value::Nine    : return 9;
            case Value::Ten     : return 10;
            case Value::Jack    : return 11;
            case Value::Queen   : return 12;
            case Value::King    : return 13;
            case Value::Ace     : return 14;
        }
    }
    int as_int(Color color){
        switch(color){
            case Color::Dimonds : return 1;
            case Color::Clubs   : return 2;
            case Color::Hearts  : return 3;
            case Color::Spades  : return 4;
        }
    }
}

class Card {

public:
    Card(Value value, Color color);

    Color getColor() const;

    Value getValue() const;

private:
    Value value;
    Color color;
};

inline bool operator==(const Card& lhs, const Card& rhs) {
    return ((rhs.getValue() == lhs.getValue()) && (rhs.getColor() == lhs.getColor()));
}
inline bool operator!=(const Card& lhs, const Card& rhs) {return !operator==(lhs,rhs);}
inline bool operator< (const Card& lhs, const Card& rhs) {
    if (as_int(lhs.getValue()) < as_int(rhs.getValue())){
        return true;
    }
    else if (as_int(rhs.getValue()) == as_int(lhs.getValue())){
        return as_int(lhs.getColor()) < as_int(rhs.getColor()) ? true : false;
    }
    else {
        return false;
    }
}
inline bool operator> (const Card& lhs, const Card& rhs) {return  operator< (rhs,lhs);}
inline bool operator<=(const Card& lhs, const Card& rhs) {return !operator> (lhs,rhs);}
inline bool operator>=(const Card& lhs, const Card& rhs) {return !operator< (lhs,rhs);}


#endif //POKER_CARD_H
\$\endgroup\$
  • \$\begingroup\$ Are there any restrictions as to which version of the standard you can use? \$\endgroup\$ – yuri May 31 '18 at 10:43
  • \$\begingroup\$ your as_int methods can be a lot shorter. You will have to reorder the enum for sutes though. You can convert the enum to an integer, then add one. \$\endgroup\$ – ctrl-alt-delor May 31 '18 at 10:46
  • \$\begingroup\$ Use of get in method names reduces readability. A method returning a not boolean should be a noun phrase, and if returning a boolean should be an adjective phrase. Expections: if where you live people say “What is the get colour?“ or “would you like get fries with that?”. \$\endgroup\$ – ctrl-alt-delor May 31 '18 at 10:49
  • \$\begingroup\$ Are you sure you need conversion from enum to int? That's fine to compare enums, not ints. \$\endgroup\$ – Victor Istomin May 31 '18 at 11:51
  • 1
    \$\begingroup\$ @ctrl-alt-delor Comments are for seeking clarification to the question, and may be deleted. Please put all suggestions for improvements in answers, even if your answer is brief. \$\endgroup\$ – 200_success May 31 '18 at 12:55
12
\$\begingroup\$

Plain enum (unlike enum class) convert implicitly to integers, so there's no need for the as_int() functions if you declare the enums in ascending order. You probably want the suits in the order ♣♦♥♠ if you want to re-use your cards for Bridge as well as Poker.

I had to make up the constructor and get methods, since they are missing from the review:

Card(Value value, Color color)
    : value{value},
      color{color}
{}

Color getColor() const { return color; };

Value getValue() const { return value; };

I also created a simple test program that returns non-zero if we break the expectations:

int main()
{
    return
        + (Card{Two, Clubs} > Card{Ace, Spades})
        + (Card{Two, Clubs} > Card{Two, Spades})
        + (Card{Two, Spades} > Card{Ace, Spades});
}

Now, we can remove the as_int():

enum Value { Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace};
enum Color { Clubs, Dimonds, Hearts, Spades};
inline bool operator==(const Card& lhs, const Card& rhs)
{
    return rhs.getValue() == lhs.getValue()
        && rhs.getColor() == lhs.getColor();
}

inline bool operator!=(const Card& lhs, const Card& rhs) { return !(lhs==rhs); }

inline bool operator< (const Card& lhs, const Card& rhs)
{
    if (lhs.getValue() < rhs.getValue()) {
        return true;
    }
    else if (rhs.getValue() == lhs.getValue()) {
        return lhs.getColor() < rhs.getColor() ? true : false;
    }
    else {
        return false;
    }
}

inline bool operator> (const Card& lhs, const Card& rhs) {return rhs < lhs;}
inline bool operator<=(const Card& lhs, const Card& rhs) {return !(lhs>rhs);}
inline bool operator>=(const Card& lhs, const Card& rhs) {return !(lhs<rhs);}

Note that this ternary conditional:

        return lhs.getColor() < rhs.getColor() ? true : false;

is exactly equivalent to this:

        return lhs.getColor() < rhs.getColor();

But we can simplify the < much further by delegating to std::tuple:

inline bool operator< (const Card& lhs, const Card& rhs) {
    return std::tuple(lhs.getValue(), lhs.getColor()) < std::tuple(rhs.getValue(), rhs.getColor());
}

In C++20, the entire set of 6 comparisons can be replaced with a single function:

public:
    auto operator<=>(const Card&) const = default;
\$\endgroup\$
  • \$\begingroup\$ This has been very insightful thank you very much! \$\endgroup\$ – Thomas Eberhard May 31 '18 at 23:22
5
\$\begingroup\$

You don't need to translate your Cards into ints to compare them.

Keep your scoped enumerations, it's a good practice. Then all you need to write is:

using Card = std::pair<Value, Color>;

because std::pair already has an overload of the comparison operators relying on the lexicographical order:

#include <utility>
#include <iostream>

enum class Value {Two, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace};
enum class Color {Hearts, Spades, Dimonds, Clubs};

int main() {

    using Card = std::pair<Value, Color>;
    Card a{Value::Six, Color::Hearts}, b{Value::Six, Color::Clubs};
    bool test = a < b;
    std::cout << std::boolalpha << test; // true
}
\$\endgroup\$
  • 1
    \$\begingroup\$ The downside of using std::pair is that the data members are now called first and second instead of value and suit, and they are now publicly-writable. \$\endgroup\$ – Toby Speight May 31 '18 at 12:19
  • \$\begingroup\$ Publicly writable isn't really a downside since the preconditions are enforced by the enum. You can't make up a "15 of hearts". first and second, I agree, is less beautiful, but again enum class prevents accidental switches so any error would be catched early and clearly. \$\endgroup\$ – papagaga May 31 '18 at 12:25
  • \$\begingroup\$ It might be a bad thing if you can change the ♥2 in your hand to a ♥A when you need it; my assumption from the original code was that it was intended as an immutable type. ...<Thinks> std::pair<const Value, const Color> should be fine, though. \$\endgroup\$ – Toby Speight May 31 '18 at 12:28
  • \$\begingroup\$ I agree with you about the rough edges of that solution, but what a gain in maintenance and coding time! \$\endgroup\$ – papagaga May 31 '18 at 12:31
  • \$\begingroup\$ Agreed - that's definitely a good review, and you've received my vote. :-) \$\endgroup\$ – Toby Speight May 31 '18 at 12:33
4
\$\begingroup\$

First if you are going to use lhs and rhs then I expect to read them on the left and right hand sides respectively (unless switching them was inherent like overloading operator> in terms of operator<).

Your switches are unnecessary

In C++, enums implicitly convert to int. If you wanted to change the underlying type you can. You can also assign a value to an individual enum and the rest will increment from there. Like so:

enum Value : unsigned { Two = 2, Three, Four, Five, Six, Seven, Eight, Nine, Ten, Jack, Queen, King, Ace };
enum Color { Hearts = 1, Spades, Dimonds, Clubs };

Now if you switched to enum class, which might be a good idea it will require the use of casts to make conversions (then if you want to overload your operators to keep your casts in one place that might be nice).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.