high card game implementation

Question

I wanted to write a simple implementation for a high card game. Basically two cards are drawn and the higher one wins. Additional rules: support for multiple decks, if same suits drawn then the higher colour wins, if same colour and suit (possible as there's multiple decks) then resolve a tie by dealing a further card to each player until a max number of tries reached. Also there exists a wild card that beats all others.

It's trivial but really I'd like to get some feedback on how it's tested. I tried to come up with a way to test the logic that depends on what rand() returns, not sure if that's the correct approach, so any feedback appreciated.

#include <stdlib.h>
#include <iostream>
#include <sstream>

#define NUM_OF_CARDS 52
#define NUM_OF_RETRIES 2

class HighCard
{
    public:
        HighCard() = default;
        ~HighCard() = default;

        bool Play( int (*f) (void) = rand, std::ostream& os = std::cout );
};

bool HighCard::Play( int (*f) (void), std::ostream& os )
{

    for (int k = 0; k < NUM_OF_RETRIES; k++)
    {
        int rank1 = -1, rank2 = -1, suit1 = -1, suit2 = -1;

        rank1 = f() % NUM_OF_CARDS + 1;
        rank2 = f() % NUM_OF_CARDS + 1;
        suit1 = f() % 4 + 1;
        suit2 = f() % 4 + 1;

        os << "rank1 is " << rank1 << " rank2 is " << rank2 << "\n";
        os << "suit1 is " << suit1 << " suit2 is " << suit2 << "\n";

        if (rank1 == rank2 && suit1 == suit2)
        {
            os << "a tie so need to draw again" << "\n";
            continue;
        }

        if (rank1 == 2 && suit1 == 2)
        {
            os << "a wild card drawn " << "\n";
            return false;
        }
        else if (rank2 == 2 && suit2 == 2)
        {
            os << "a wild card drawn " << "\n";
            return true;
        }

        if (rank1 != rank2)
            return ( rank1 < rank2 );
        else
            return ( suit1 < suit2 );
    }

    os << "a tie repeated too many times..." << "\n";

    return true;
}

// function used for testing instead of rand() 
// ints passed in instantiation will be returned in subsequent calls
template<int n1, int n2, int n3, int n4,
         int n5 = -1, int n6 = -1, int n7 = -1, int n8 = -1>
int drawNums()
{
    static int i = -1;
    i++;
    if (i % 8 == 0)
        return n1;
    else if (i % 8 == 1)
        return n2;
    else if (i % 8 == 2)
        return n3;
    else if (i % 8 == 3)
        return n4;
    else if (i % 8 == 4)
        return n5;
    else if (i % 8 == 5)
        return n6;
    else if (i % 8 == 6)
        return n7;
    else 
        return n8;
}

void test_win ()
{
    HighCard card;
    std::ostringstream oss;

    oss.str("");

    // rank1 > rank2, suit1 < suit2
    assert(card.Play( drawNums<22,3,0,1>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 23 rank2 is 4\n"
                               "suit1 is 1 suit2 is 2\n");

    oss.str("");

    // rank1 > rank2, suit1 > suit2
    assert(card.Play( drawNums<22,3,1,0>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 23 rank2 is 4\n"
                               "suit1 is 2 suit2 is 1\n");

    oss.str("");

    // rank1 > rank2, suit1 = suit2
    assert(card.Play( drawNums<22,3,0,0>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 23 rank2 is 4\n"
                               "suit1 is 1 suit2 is 1\n");

    oss.str("");

    // rank1 < rank2, suit1 < suit2
    assert(card.Play( drawNums<3,22,0,1>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 4 rank2 is 23\n"
                               "suit1 is 1 suit2 is 2\n");

    oss.str("");

    // rank1 < rank2, suit1 > suit2
    assert(card.Play( drawNums<3,22,1,0>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 4 rank2 is 23\n"
                               "suit1 is 2 suit2 is 1\n");

    oss.str("");

    // rank1 < rank2, suit1 = suit2
    assert(card.Play( drawNums<3,22,0,0>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 4 rank2 is 23\n"
                               "suit1 is 1 suit2 is 1\n");

    oss.str("");

    // rank1 = rank2, suit1 < suit2
    assert(card.Play( drawNums<22,22,0,1>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 23 rank2 is 23\n"
                               "suit1 is 1 suit2 is 2\n");

    oss.str("");

    // rank1 = rank2, suit1 > suit2
    assert(card.Play( drawNums<22,22,1,0>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 23 rank2 is 23\n"
                               "suit1 is 2 suit2 is 1\n");
}

void test_wildcard()
{
    HighCard card;
    std::ostringstream oss;

    oss.str("");

    // rank1 < rank2, suit1 < suit2
    assert(card.Play( drawNums<1,3,1,3>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 2 rank2 is 4\n"
                               "suit1 is 2 suit2 is 4\n"
                               "a wild card drawn \n");

    oss.str("");

    // rank1 > rank2, suit1 > suit2
    assert(card.Play( drawNums<1,0,1,0>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 2 rank2 is 1\n"
                               "suit1 is 2 suit2 is 1\n"
                               "a wild card drawn \n");

    oss.str("");

    // rank1 > rank2, suit1 > suit2
    assert(card.Play( drawNums<3,1,3,1>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 4 rank2 is 2\n"
                               "suit1 is 4 suit2 is 2\n"
                               "a wild card drawn \n");

    oss.str("");

    // rank1 < rank2, suit1 < suit2
    assert(card.Play( drawNums<0,1,0,1>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 1 rank2 is 2\n"
                               "suit1 is 1 suit2 is 2\n"
                               "a wild card drawn \n");

}

void test_retries_limit()
{
    HighCard card;
    std::ostringstream oss;

    oss.str("");

    assert(card.Play( drawNums<2,2,2,2,  2,2,2,2>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 3 rank2 is 3\n"
                               "suit1 is 3 suit2 is 3\n"
                               "a tie so need to draw again\n"
                               "rank1 is 3 rank2 is 3\n"
                               "suit1 is 3 suit2 is 3\n"
                               "a tie so need to draw again\n"
                               "a tie repeated too many times...\n");

    // wild card
    oss.str("");

    assert(card.Play( drawNums<1,1,1,1,  1,1,1,1>, oss ) == true);
    assert(oss && oss.str() == "rank1 is 2 rank2 is 2\n"
                               "suit1 is 2 suit2 is 2\n"
                               "a tie so need to draw again\n"
                               "rank1 is 2 rank2 is 2\n"
                               "suit1 is 2 suit2 is 2\n"
                               "a tie so need to draw again\n"
                               "a tie repeated too many times...\n");
}

void test_retries()
{
    HighCard card;
    std::ostringstream oss;

    oss.str("");

    assert(card.Play( drawNums<2,2,2,2  ,23,2,2,2>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 3 rank2 is 3\n"
                               "suit1 is 3 suit2 is 3\n"
                               "a tie so need to draw again\n"
                               "rank1 is 24 rank2 is 3\n"
                               "suit1 is 3 suit2 is 3\n");

    // wild card
    oss.str("");

    assert(card.Play( drawNums<1,1,1,1, 1,2,1,3>, oss ) == false);
    assert(oss && oss.str() == "rank1 is 2 rank2 is 2\n"
                               "suit1 is 2 suit2 is 2\n"
                               "a tie so need to draw again\n"
                               "rank1 is 2 rank2 is 3\n"
                               "suit1 is 2 suit2 is 4\n"
                               "a wild card drawn \n");
}


int main (int argc, char **argv)
{
    srand (time(NULL));

    test_win();
    test_wildcard();
    test_retries();
    test_retries_limit();


    return 0;
}

Answer 1

Ok, let's start with basics.

class HighCard doesn't make much sense. Make it a namespace.

The method bool HighCard::Play( int (*f) (void) = rand, std::ostream& os = std::cout ); isn't convenient by any means. Supplementing a function pointer is very old fashioned (C style not a C++ way) and a poor practice in general.

You'd better use std::function<int(void)> make HighCard::Play into a template that accepts a callable as the first parameter.

The reason why function pointer is not good is because it isn't general enough - rand is a very poor random function and it is preferable to use std::default_random_device which is a class with data and one cannot use it via function pointers. std::function<int(void)> can wrap it but calling it is generally slow, so for the code to be efficient it is best to use template callable and wrap whatever needed into a lambda/closure.

This would also solve the problem with the ridiculous template template<...> int drawNums(). Once your method accepts a callable just make a simple structure that holds a vector of prefixed results and passes them.

rank1 = f() % NUM_OF_CARDS + 1;
rank2 = f() % NUM_OF_CARDS + 1;
suit1 = f() % 4 + 1;
suit2 = f() % 4 + 1;

This is not a good way to generate random numbers. Use std::uniform_int_distribution to properly randomize values instead of % operation. It requires a random engine to work, so it best be used inside the function. So make the function/callable in format std::function<int(int)> and use it as

rank1 = f(NUM_OF_CARDS);
rank2 = f(NUM_OF_CARDS);
suit1 = f(4);
suit2 = f(4);

so f(val) generates number between 1 and val including.

Do not use MACROS. Use constexpr or enum instead for NUM_OF_CARDS and NUM_OF_RETRIES.

I think you can potentially make HighCard::Play into a constexpr conditionally on the input (by also removing the logs...). In which case you can perform static_assert for the tests instead of assert. This means that you can compile-time run the test instead of relying on run-time verification.

Also, about logs, HighCard::Play shouldn't quite print anything. Instead it should send results (an enum of current state) to some other class which should perform printing or whatever. This way HighCard::Play will be cleaner and printing is delegated to some other code.

Answer 2

My main issue with the game is that you randomly generate cards:

    rank1 = f() % NUM_OF_CARDS + 1;
    rank2 = f() % NUM_OF_CARDS + 1;
    suit1 = f() % 4 + 1;
    suit2 = f() % 4 + 1;

This is not like real life where the probability of two people getting the same card is zero (here the chances are small but they exist).

A better technique would be to generate a pack of cards and then randomly shuffle the cards. Then select cards from the top of the pack (selected cards can not be re-selected you can just keep track of the current top). When you want to reset simply re-shuffle and reset the top to zero. This way you guarantee that you get a fair chance of any card but you can not have two cards the same.

---

Talking about random numbers.

The old C style random number generator:

srand()
rand()

They are easy to use (also easy to use incorrectly).

Assuming f() maps to rand() then:

 f() % NUM_OF_CARDS + 1;

Is not correct (just check stack-overflow for answers about this (there are some violent debates)). Suffice to say not all the numbers in [0..NUM_OF_CARDS] are equally as likely (the lower numbers have a slightly higher probability than the higher numbers).

Also it has been shown that rand() is not that random. There are papers on the subject.

Rather than work out how to do all the correctly you can simply switch to the C++ functionality:

 // Only create this once in main.
 // Then pass it to where it needs to go.
 std::default_random_engine generator;

 std::uniform_int_distribution<int> suit(1,4);
 std::uniform_int_distribution<int> rank(1,13);

 rank1 = rank(generator);
 suit1 = suit(generator);
 //etc

Or probably better:

 std::default_random_engine generator;

 std::uniform_int_distribution<int> card(0,51);

 card1 = card(generator);
 rank1 = (card % 13) + 1; // you use 1-13 hence the +1
 suit1 = (card / 13) + 1; // you use 1-4. hence the +1

---

This is an interesting comment:

// function used for testing instead of rand() 
// ints passed in instantiation will be returned in subsequent calls

yes testing becomes hard when you use random numbers. But the rand functionality is designed with that in mind. You can make sure you get the same set of random numbers by seeding the generator with a specific start point.

srand(10);   // The random number will start from the seed of 10.
             // starting here will always generate the same set of random
             // numbers. So if you want your unit tests to always run the
             // same way simply seed the rand number generator before
             // running the test with a specific value (you will get the
             // same sequence of randoms).

Same happens for the C++ version:

std::default_random_engine generator(10);

But saying all that I do like your number generator idea.

---

In C++ we frown upon #define

#define NUM_OF_CARDS 52
#define NUM_OF_RETRIES 2

Nearly all use cases for #define have been replaced by language (rather than pre-processor) features. Prefer to use language features where they exist.

static constexpr int NumOfCards   = 52; // Note all uppercase is still macro names so avoid them.
static constexpr int NumOfRetries = 2;

Here these values are correctly typed.

---

Better implementation:

if you have a lot if () else if else if else if.

template<int n1, int n2, int n3, int n4,
         int n5 = -1, int n6 = -1, int n7 = -1, int n8 = -1>
int drawNums()
{
    static int i = -1;
    i++;
    if (i % 8 == 0)
        return n1;
    else if (i % 8 == 1)
        return n2;
    else if (i % 8 == 2)
        return n3;
    else if (i % 8 == 3)
        return n4;
    else if (i % 8 == 4)
        return n5;
    else if (i % 8 == 5)
        return n6;
    else if (i % 8 == 6)
        return n7;
    else 
        return n8;
}

You may want to look at using a switch statement.

template<int n1, int n2, int n3, int n4,
         int n5 = -1, int n6 = -1, int n7 = -1, int n8 = -1>
int drawNums()
{
    static int i = -1;
    i++;

    switch(i % 8) {
        case 0: return n1;  // Though i would have numbered from n0 - n7
        case 1: return n2;  // To make this more logical.
        case 2: return n3;
        case 3: return n4;
        case 4: return n5;
        case 5: return n6;
        case 6: return n7;
        case 7: return n8;
    }
}

But we can go one better than that.
Why do we need a switch when we simply use an array and look up the value.

template<int n1, int n2, int n3, int n4,
         int n5 = -1, int n6 = -1, int n7 = -1, int n8 = -1>
int drawNums()
{
    static int result[] = {n1, n2, n3, n4, n5, n6, n7, n8};
    static int i = -1;
    i++;
    return result[i % 8];
}

But let's take this a step further. We don't need to limit ourselves to 8 values we can simply allow as many values as we like by using var arg template arguments:

template<int... Vals>
int drawNums()
{
    static int result[] = {Vals...};
    static int i = -1;
    i++;
    return result[i % sizeof...(Vals)];
}

---

Remember C is not the same language as C++. So pefer not to use C header files where eqivelent BUT correctly namespaced for C++ versions exist in C++

#include <stdlib.h>
----
#include <cstdlib>    // Functions are placed in the `std` namespace.
                      // Helps avoid collisions.

---

One of the obscure corner cases where C and C++ actually differ is the use of a void parameter.

 func();
 func(void);

In C these have different meanings.
In C++ they mean exactly the same thing. As a result in C++ we don't bother with the extra void parameter as it does not convey any extra meaning and is just fluff.

Thus you're declarations for function pointers:

 int (*f) (void)

That void has no menaing in this context. This is simpler to write (and more commonly written as):

 int (*f)()

Though to be honest even this is rare. We would normally wrap this in a std::function object that allows us to pass other types (not just functions)

  bool HighCard::Play( int (*f) (void), std::ostream& os )

I would write like this:

  bool HighCard::Play(std::function<int()> const& action, std::ostream& os)

Now I can pass a function pointer a functor or even a lambda.

  highCard.Play([](){return 5;}, std::cout);

Answer 3

First, I suggest using a unit test framework.

Test Cases

You test multiple test cases in one function. Write a test in the format of "Arrange Act Assert". You should have a single act - a single call to HighCard::Play in each test.

Assert

You are asserting the output of the game. This makes the tests fragile because a change in the presentation will break tests.

Separate logic from the presentation and test the logic. The tests will become shorter and clear.

Random

• You should not have a test code on your production code.

• A way to handle random logic in tests is to replace it with mocks.

test_win and test_wildcard

Separate the logic of who is the winning card to another function like this WinStatus whoWin(rank1,suit1,rank2,suit2,card2).

Now, you can test whoWin and there are no calls to rand.Also, it creates documentation for the rules.

Retries tests

I see a few options here:

1. Mock rand calls. Wrap the calls to rand with a class and use mock to return different values for each call. This is what you did but with mocks.

2. Mock takeCard. The following lines are the logic of taking a card from the deck:

   rank1 = f() % NUM_OF_CARDS + 1;
suit1 = f() % 4 + 1;

   Use mock to return different values for each call. Having a Card class will help here.

3. Mock the result of whoWin. You still call rand in tests but it doesn't affect anything.

I prefer the second option because it tests more logic in a single test.