Determine playing card

Question

I would like a code review on the coding I did for my assignment. The reason I have asked here is because my lecturer has no intention of going through (Don't ask me why, I asked before and he said no..) and I have no idea if I am doing it right or not.. All my other classmates seems to have different answers too, some have deck class, shuffle function etc..

Anyway the question is asking the user to design and implement a class that represent a playing card in which it can be used to play card games. Consider what information you needs to be stored in a card and what you man want to do with a card (accessors/mutators).

In this assignment, only 2 files are required - 1 header and 1 implementation file. Kindly see the following:

Header file

#ifndef  CARD_H
#define  CARD_H
#include<string>

using namespace std;

class card
{
    private:
        string cardSuit;
        string cardValue;

    public:
        // Constructor
        string displayCard(string, string);

        // Member Functions
        string getSuit();
        string getValue();

        // Default
        card();

};
#endif

Implementation File

#include <iostream>
#include <string>
#include <ctime>
#include <cstdlib>
#include "card.h"

using namespace std;


card::card()
{
    srand(time(0));
    cardSuit = getSuit();
    cardValue = getValue();
}


string card::displayCard(string suit, string value)
{
    return cardSuit + " of " + cardValue;
}


string card::getSuit()
{

    string arrSuits[] = {"HEARTS", "DIAMONDS", "CLUBS", "SPADES"};

    // 1 out of 4 suits is choose randomly
    int randSuits = rand() % 4;
    return arrSuits[randSuits];
}

string card::getValue()
{
    string arrValues[] = {"ACE", "ONE", "TWO", "THREE", "FOUR", "FIVE", "SIX", "SEVEN", "EIGHT", "NINE", "TEN", "JACK", "QUEEN", "KING"};

    // 1 out of 13 values is choose randomly
    int randValues = rand() % 13;

    return arrValues[randValues];
}

As I have only started out in C++, do pardon me if my coding format is wrong etc.

Answer 1

In object-oriented programming, a card object is supposed to represent a card. Your card, though… I'm not sure what it represents! If anything, it's the ultimate chameleon card: every time you call .getSuit() or .getValue() on it, it reports a different value!

The card's identity, therefore, must be determined in the constructor, and it should never change thereafter. All three methods should be declared const.

Furthermore, it's a bad idea to have the card's identity be randomly assigned in the constructor. How would you create a deck of 52 different cards, if you couldn't instantiate each one deterministically?

The displayCard() method makes no sense — why should you pass in suit and value as parameters, which will then be ignored?

using namespace std; defeats the purpose of the std namespace. It's sloppy practice in general. In particular, you should never do that in a header file, because any .cpp file that includes that header will have its namespace contaminated as well.

Your comments are wrong: displayCard() is not a constructor.

So, the header file should look more like this:

#ifndef  CARD_H
#define  CARD_H
#include <string>

class Card
{
    public:
        // Constructor
        Card(const std::string &suit, const std::string &value);

        // Member Functions
        std::string getSuit() const;
        std::string getValue() const;
        std::string display() const;

    private:
        std::string suit, value;
};
#endif

I'll leave it to you to figure out what the implementation should look like.

Answer 2

Adding to what 200_success answered, I think it would improve the implementation if you create a enum to specify the valid suits (you can also do the same with the values). This way when creating a card you are forced to used a valid Suit.

class Card
{
public:
    enum Suits
    {
         E_SPADES,
         E_HEARTS,
         E_DIAMONDS,
         E_CLUBS
    }
    // Constructor
    Card(const Suits &suit, const std::string &value);

    // Member Functions
    Suits getSuit() const;
    std::string getValue() const;
    std::string display() const;

private:
    Suits suit; 
    std::string value;
};