2
\$\begingroup\$

I'm trying to make a card game, using OOP style. I decided to make suit a class rather than an enum as I would first have done. How am I doing so far? Am I over-complicating things by making it a class?

namespace Cards
{
class Suit
{
public:
    Suit();
    ~Suit();

    std::string GetName(bool plural, bool capital) const;
    bool IsRed() const;
    bool IsBlack() const;
    bool IsDifferentColour(const Suit& other) const;
    bool IsSameColour(const Suit& other) const;
    bool operator == (const Suit& other) const;
    bool operator != (const Suit& other) const;
    bool operator < (const Suit& other) const;

    static const Suit hearts, diamonds, clubs, spades;

private:
    enum Type
    {
        Type_Null,
        Type_Hearts,
        Type_Diamonds,
        Type_Clubs,
        Type_Spades
    } type;

    explicit Suit(Type type);
};
}

const Cards::Suit Cards::Suit::hearts(Type_Hearts);
const Cards::Suit Cards::Suit::diamonds(Type_Diamonds);
const Cards::Suit Cards::Suit::clubs(Type_Clubs);
const Cards::Suit Cards::Suit::spades(Type_Spades);

Cards::Suit::Suit()
    : type(Type_Null)
{}

Cards::Suit::Suit(Type type)
    : type(type)
{}

Cards::Suit::~Suit()
{}

std::string Cards::Suit::GetName(bool plural, bool capital) const
{
    std::string name;

    switch (type)
    {
    case Type_Hearts:
        {
             name = "Heart";
        }
        break;

    case Type_Diamonds:
        {
            name = "Diamond";
        }
        break;

    case Type_Clubs:
        {
            name = "Club";
        }
        break;

    case Type_Spades:
        {
            name = "Spade";
        }
        break;

    default:
        {
            assert(false);
        }
        break;
    }

    if (! capital)
    {
        name[0] = static_cast<std::string::value_type>(::tolower(name[0]));
    }

    if (plural)
    {
        name += "s";
    }

    return name;
}

bool Cards::Suit::IsRed() const
{
    bool red;

    switch (type)
    {
        case Type_Hearts:
        {
             red = true;
        }
        break;

    case Type_Diamonds:
        {
            red = true;
        }
        break;

    case Type_Clubs:
        {
            red = false;
        }
        break;

    case Type_Spades:
        {
            red = false;
        }
        break;

    default:
        {
            assert(false);
            // prevent compiler warning
            red = false;
        }
    }

    return red;
}

bool Cards::Suit::IsBlack() const
{
    return ! IsRed();
}

bool Cards::Suit::IsDifferentColour(const Suit& other) const
{
    return this->IsRed() != other.IsRed();
}

bool Cards::Suit::IsSameColour(const Suit& other) const
{
    return this->IsRed() == other.IsRed();
}

bool Cards::Suit::operator == (const Cards::Suit& other) const
{
    return this->type == other.type;
}

bool Cards::Suit::operator != (const Cards::Suit& other) const
{
    return ! (*this == other);
}

bool Cards::Suit::operator < (const Cards::Suit& other) const
{
    return this->type < other.type;
}
\$\endgroup\$
3
\$\begingroup\$

If the destructor does nothing remove it:

Cards::Suit::~Suit()
{}

We can shorten that switch statement to make it readable.

    switch (type)
    {
        case Type_Hearts:   name = "Heart";  break;
        case Type_Diamonds: name = "Diamond";break;
        case Type_Clubs:    name = "Club";   break;
        case Type_Spades:   name = "Spade";  break;
        default:            assert(false);   break;
    }

Or we can replace it with a look up:

    static std::string  names[] = {"NULL", "Heart", "Diamond", "Club", "Spade"};
    if (type == Type_Null)
    {    assert(false); // Or throw.
    }
    name =  names[type];

If you want more checking:

    static std::vector<std::string>  names = {"Heart", "Diamond", "Club", "Spade"};
    name =  names.at(type-1); // will throw on bad value.

This seems a bit over keen.

        name[0] = static_cast<std::string::value_type>(::tolower(name[0]));

std::string::value_type by definition is char.
tolower() may return an int unless you pass in an EOF you will get a character back.

I would have just done:

        name[0] = ::tolower(name[0]);

In this switch use fall throug:

switch (type)
{
    case Type_Hearts:    /* fall through */
    case Type_Diamonds:  return true;

    case Type_Clubs:     /* fall through */
    case Type_Spades:    return false;
    default:  assert(false);
        // prevent compiler warning
        return true;
}

Is A Null_Suit the same as another Null_Suit?
in double a NaN is not equal to another NaN you may want that kind of functionality.

bool Cards::Suit::operator == (const Cards::Suit& other) const
{
    return this->type == other.type;
}
\$\endgroup\$
2
\$\begingroup\$

Yes I think you are over complicating things. In C++11 I would use a strongly typed enum:

enum class Suite {Hearts,
                  Diamonds,
                  Clubs,
                  Spades};

I might also have considered a simple class hierarchy

namespace suite
{
  class SuiteBase{...};
  class Hearts : public SuiteBase {...};
  class Diamonds : public SuiteBase {...};
  class Clubs : public SuiteBase {...};
  class Spades : public SuiteBase {...};
}

Then create needed operations between suites. Preferably using some double dispatch technique to be able to compare SuiteBase pointers.

\$\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.