4
\$\begingroup\$

I'm currently in the process of refactoring an old poker game which violates many of the SOLID principles.

I was trying to use the TDD approach to refactoring and I found myself having to hard code a player's hand every time I wrote a unit test for a specific poker hand check.

For example, here are the unit tests for a one pair poker hand check:

    [Test]
    public void OnePair_IsLowPair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Ace),
            new CardSpade(Rank.Ace),
            new CardDiamond(Rank.Five),
            new CardHeart(Rank.Jack),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsLowerMiddlePair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Eight),
            new CardSpade(Rank.Ace),
            new CardDiamond(Rank.Ace),
            new CardHeart(Rank.Jack),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsHigherMiddlePair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Jack),
            new CardSpade(Rank.Queen),
            new CardDiamond(Rank.Ace),
            new CardHeart(Rank.Ace),
            new CardClub(Rank.Three)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

    [Test]
    public void OnePair_IsHighPair_ReturnsTrue()
    {
        SuperCard[] testHand =
        {
            new CardClub(Rank.Jack),
            new CardSpade(Rank.Deuce),
            new CardDiamond(Rank.Six),
            new CardHeart(Rank.Ace),
            new CardClub(Rank.Ace)
        };

        bool result = PokerHandEvaluator.OnePair(testHand);

        Assert.That(result == true);

    }

This is what the class diagram (at the project's current stage) looks like: enter image description here

My question is how I should go about extracting a method (or methods) out of the testHand initialization.

\$\endgroup\$
  • 1
    \$\begingroup\$ Maybe make a "CreateHand()" function that takes a string like "AcAs5dJh3c"? \$\endgroup\$ – AShelly Mar 27 at 5:51
  • \$\begingroup\$ A CreateHand() is essentially what I'm trying to go for here but I'm not sure how to implement your idea. Can you please share some pseudocode or code? \$\endgroup\$ – Asel S Mar 27 at 5:57
  • 2
    \$\begingroup\$ To those who have put this question on hold, could you clarify how it lacks concrete context? \$\endgroup\$ – Asel S Mar 27 at 21:55
  • 2
    \$\begingroup\$ @AselS My experience as a moderator is that that reason is a bit abused, I don't see a reason for why it was put on hold for that reason so I have reopened it. The only possible explanation would be that people would like to see more of your PokerHandEvaluator implementation, but to me it's clear that it's your tests that are up for review and not the code that you are testing. \$\endgroup\$ – Simon Forsberg Mar 28 at 18:02
  • 2
    \$\begingroup\$ Thank you for clarifying Simon. I would have happily included more code if the users who marked the question as on hold, for lack of context, would have clarified which aspects of the code they wanted to see. \$\endgroup\$ – Asel S Mar 28 at 20:01
1
\$\begingroup\$

One thing that would help with reducing duplication is to use parameterized tests.

Additionally, you might want to have a enum with the different hand types instead of - or in addition to - having one method for each hand type, such as your current PokerHandEvaluator.OnePair.

\$\endgroup\$
  • \$\begingroup\$ I like this idea but I'm having some difficulty passing in my arrays as parameters. I tried to do [TestCase(new SuperCard[] {new CardClub(Rank.Ace)...} ) ] but I'm getting an error saying that the attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type. How would you construct a single OnePair test with several test cases that accept SuperCard array as parameters? I should mention that I recently got into unit testing. \$\endgroup\$ – Asel S Mar 28 at 20:49
  • \$\begingroup\$ Also, could you elaborate on why you disagree with Matt's static idea method? Is it because that would also mean repetition? And could you clarify what you mean by having an enum with different hand types? Since I already have a PokerHand class with the various hand types, do you mean to say that I should have the variants of them as well? For example, OneHandLow, OneHandHigh, etc. \$\endgroup\$ – Asel S Mar 28 at 20:50
  • \$\begingroup\$ @AselS You can specify test data other ways: github.com/nunit/docs/wiki/TestCaseSource-Attribute . I dislike Matt's static method because I think a parameterized test is better, and because you would need one static method for every test. With different hand types I mean OnePair, TwoPair, ThreeOfAKind, etc. and have one method that returns which hand it is, see PokerHandType in my old poker hand evaluator (Java) \$\endgroup\$ – Simon Forsberg Mar 28 at 21:31
  • \$\begingroup\$ Thank you for clarifying. I managed to implement the TestCaseSource after looking at some other resources and I edited my post to reflect my attempt. Could you please take a look at it and see whether that is what you had in mind? If not, what would you change further? \$\endgroup\$ – Asel S Mar 29 at 0:48
  • \$\begingroup\$ Just to address the poker hand types, I do have a class called PokerHand (which is just an enum) that holds those types, and I do have a method called EvaluatePokerHand() in my PokerHandEvaluator class that returns the enum equivalent of the hand (these are reflected in the class diagram). I have a handful of tests for this method as well, that has the same problem with initializing, which I've posted a link too in my edit. \$\endgroup\$ – Asel S Mar 29 at 0:54
1
\$\begingroup\$

IMO I'd join all of your tests into one. And just rotate through them.

private static void Rotate<T>(List<T> list) {
    T first = list[0];
    list.RemoveAt(0);
    list.Add(first);
}

[Test]
public void TestOnePairs()
{
    List<SuperCard> testHand = new List<SuperCard> {
        new CardClub(Rank.Ace),
        new CardSpade(Rank.Ace),
        new CardDiamond(Rank.Two),
        new CardHeart(Rank.Three),
        new CardClub(Rank.Four)
    };

    for (let i=0; i < 5; i++) {
        Assert.That(PokerHandEvaluator.OnePair(testHand.ToArray()));
        Rotate(testHand);
    }
}

If this were Python I'd suggest changing List<SuperCard> to List<Tuple<Suit, Rank>>. However IIRC C# doesn't like Tuples too much. And so you may want to make them List<Card>, but pass both of the enums to the card at instantiation.


I should note that this doesn't test all combinations that the pairs can be. And so you should really use something like Pythons itertools.combinations.

This is as the following pseudocode of OnePair would be incorrect, but would pass your tests.

def OnePair(values):
    prev = values[-1]
    for curr in values:
        if curr.rank == prev.rank:
            return True
        prev = curr
    return False
\$\endgroup\$
  • \$\begingroup\$ I like this idea because it would solve the variant test cases on One Pairs (as well as a few other hands) but that would still leave me with several tests that still have to initialize a unique SuperCard array. Is there a way to solve that issue as well using this method? \$\endgroup\$ – Asel S Mar 28 at 21:14
  • 1
    \$\begingroup\$ @AselS For me to comment on these other tests, you'd have to provide them to us. And so you'd need to ask a new question with these tests in it. \$\endgroup\$ – Peilonrayz Mar 28 at 21:16
1
\$\begingroup\$

I decided to post a self-answer as suggested by t3chbot because the following code example is just an attempt at following Simon's suggestion; I haven't fully incorporated the code as I have not managed to resolve my original question. I hope this is okay.

public class OnePairTestDataSource : IEnumerable
    {
        public IEnumerator GetEnumerator()
        {
            yield return new SuperCard[]
            {
                new CardClub(Rank.Ace),
                new CardSpade(Rank.Ace),
                new CardDiamond(Rank.Five),
                new CardHeart(Rank.Jack),
                new CardClub(Rank.Three)
            };

            ...
        }

    [TestCaseSource(typeof(OnePairTestDataSource))]
        public void OnePair_IsOnePair_ReturnsTrue(SuperCard[] pTestHand)
        {
            bool result = PokerHandEvaluator.OnePair(pTestHand);
            Assert.That(result == true);
        }

Even though I like Simon's suggestion of using parameterized tests, I noticed that I still have to do some repetition in order to initialize an array that I want to test. Is this what you had in mind Simon? If not could you clarify with a code example perhaps?


I tried using a parameterized test source as well. Here is another implementation which I like better than the one above and my original one, but still leaves me with the same problem:

static IEnumerable<SuperCard[]> GetTestHand(PokerHand pPokerHand)
{
   if (pPokerHand == PokerHand.OnePair)
   {
     yield return new SuperCard[]
     {
       new CardClub(Rank.Ace),
       new CardSpade(Rank.Ace),
       new CardDiamond(Rank.Five),
       new CardHeart(Rank.Jack),
       new CardClub(Rank.Three)
     };

     ...
   }
}

[TestCaseSource(nameof(GetTestHand), new object[] { PokerHand.OnePair })]
public void OnePair_IsOnePair_ReturnsTrue(SuperCard[] pTestHand)
{
   bool result = PokerHandEvaluator.OnePair(pTestHand);
   Assert.That(result == true);
}
\$\endgroup\$
0
\$\begingroup\$

Not sure if there is any advantage of having 4 classes (CardClub, CardSpade, CardHeart, CardDiamond) with one property Rank,

compared to

a Card class with 2 properties, Suit (of Type Suit enum) and Rank (of Type Rank enum).

Have a CardFactory class, that has a static method

Card CreateCard(Suit suit, Rank rank) => new Card(suit, rank);

We could have a HandTestDataGenerator class with a static method for generating test hands:

public static IList<Card> CreateCards(Dictionary<Suit, List<Rank> cards) =>
    cards.SelectMany(x => x.Value.Select(rank => new Card(x.Key, rank));

Now for each test data, create a static method in HandTestDataGenerator like:

IList<Card> GenerateLowPairHand()
{
    var lowPairHand = new Dictionary<Suit, List<Rank>>();
    lowPairHand.Add(Suit.Club, new List<Rank>{ Rank.Ace, Rank.Three });
    //Add other suits and ranks here
    return CreateCards(lowPairHand);
}

From TestMethod, call

SuperCard[] testHand = HandTestDataGenerator.GenerateLowPairHand();

\$\endgroup\$
  • \$\begingroup\$ I couldn't follow the Dictionary implementation but creating a class with static methods for each test data seems like a good approach. As to why it had four classes for cards with rank, it was a design constraint on an assignment. \$\endgroup\$ – Asel S Mar 27 at 21:58
  • \$\begingroup\$ For each card, you need a suit and rank. for your testdata you could have Club-Ace and Club-Three; hence thought of having a dictionary with Suit as Key and Ranks as a List. \$\endgroup\$ – Matt.G Mar 27 at 22:05
  • \$\begingroup\$ I agree about having one card class with two properties, I disagree about having one static method for each test-data. \$\endgroup\$ – Simon Forsberg Mar 28 at 18:08

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.