Loop combinations of byte for Poker simulator

Question

In (this) Poker you have 2 hole cards that are combined with 5 shared cards on the board. Each player makes the best 5 card hand from the seven. Believe it or not that is over 2 billion runs to do them all.

I'm trying to create statistics for 2 known hole cards against random boards and random hands for a single opponent using Monte Carlo for like 20 million runs.

First get sets of 7 random for board plus 1 opponent from 50 (known 2 hole cards are removed). From those 7 remove all 2 card combinations (21) to represent the opponent's hole cards. The idea is better statistics with an exhaustive examination of the 7 rather than a random 5 and a random 2. Computation on the random 5 plus random 2 is more efficient as you only need to run the known hand once for each random 5.

byte? skip1 = null;
byte? skip2 = null;
List<byte> deck50seven = null;
byte nextSevenLastStart = 42;
private IEnumerable<byte> NextSevenA(byte Skip1, byte Skip2)
{
    if (Skip1 == Skip2)
        throw new ArgumentOutOfRangeException("skip1 == skip2");
    if (Skip1 > 51 || Skip2 > 51 || Skip1 < 0 || Skip2 < 0)
        throw new ArgumentOutOfRangeException("51 0");
    if(skip1 == null)
    {
        skip1 = Skip1;
        skip2 = Skip2;
        deck50seven = new List<byte>(50);
        for(byte b = 0; b < 52; b++)
        {
            if (b == Skip1 || b == Skip2)
                continue;
            deck50seven.Add(b);
        }
    }
    else
    {
        if (Skip1 != skip1 || Skip2 != skip2)
            throw new ArgumentException("skippy");
    }
    if (nextSevenLastStart >= 42)
    {
        nextSevenLastStart = 0;
        // Yates shuffle
        for (int i = 49; i >= 1; i--)
        {   // int for indexing is faster (I think)
            int j = rand.Next(i + 1);  // it can get swapped with itself
            if (j != i)
            {   // exchange values
                byte curVal = deck50seven[i];
                deck50seven[i] = deck50seven[j];
                deck50seven[j] = curVal;
            }
        }
    }
    else
        nextSevenLastStart += 7;
    return deck50seven.GetRange(nextSevenLastStart, 7);
}
private IEnumerable<List<byte>> NextSevenAsFiveTwo(List<byte> input)
{
    // 21 ways to take 2 from 7 
    // this is going to seem strange but need every combination of 2 to be the last 2
    if (input.Count() != 7)
        throw new ArgumentOutOfRangeException("input.Count() != 7");
    List<byte> output = new List<byte>();
    for (byte i = 0; i <= 5; i++)
    {
        for (byte j = (byte)(i + 1); j <= 6; j++)
        {
            output.Clear();
            for (byte k = 0; k < 7; k++)
                if (k != i && k != j)
                    output.Add(input[k]);
            output.Add(input[i]);
            output.Add(input[j]);
            yield return output;
        }
    }
}

can use this for test

for (int i = 0; i < 10; i++)
{
    List<byte> nextSevenA = new List<byte>(NextSevenA(11, 12));
    Debug.WriteLine("Base " + string.Join(", ", nextSevenA));
    foreach(List<byte> ss in NextSevenAsFiveTwo(nextSevenA))
        Debug.WriteLine("     Subset " + string.Join(", ", ss));
}

Answer 1

private IEnumerable<byte> NextSevenA(byte Skip1, byte Skip2)

• Method arguments should be named using camelCase casing. In this way you wouldn't mistake them for being properties.

  If you need the PascalCase casing of the arguments to distinguish them from your local variables, you should either change their names or use this for use with class level variables.

• Omitting braces although they might be optional, like for single lined if statements, can lead to serious bugs which are hard to find. I would like to encourage you to always use them. This not only makes your code less error prone but also more readable because it looks more structured.

• throwing an ArgumentOutOfRangeException is a good thing but the error message should be more expressive. "51 0" isn't telling really much.

• You should validate

      if (Skip1 != skip1 || Skip2 != skip2)
          throw new ArgumentException("skippy");  
  

  earlier to make it clear where the validation happens.

• by extracting the generation of the byte values of deck50seven to a separate method returning an IEnumerable<byte> the method could be shorter.

  private static IEnumerable<byte> GetByteRange(byte start, byte end)
  {
      for (byte b = start; b < end; b++)
      {
          yield return b;
      }
  }  
  

  After introducing a private const int MaxCardsInDeck = 52 this could be called like

  deck50seven = GetByteRange(0, MaxCardsInDeck).Except(new byte[] { Skip1, Skip2 }).ToList();  
  

• By changing the way you handle nextSevenLastStart you could get rid of the else part at the end. I say could because I don't know if you somewhere else using that variable. The way I would do this is like so

  nextSevenLastStart += 7;
  if (nextSevenLastStart >= 49)
  {
      //shuffling now
  }
  

  If this isn't an option you should consider to invert the condition if (nextSevenLastStart >= 42) and return early which saves you one level of indentation.

• The shuffeling of the deck50seven should be extracted to a separate method like so

  private static Random rand = new Random();
  private static void Shuffle(List<byte> items, int upperBound=42,int lowerBound=1)
  {
      for (int i = upperBound; i >= lowerBound; i--)
      {   
          int j = rand.Next(i + 1);  
          if (j != i)
          {  
              byte currentValue = items[i];
              items[i] = items[j];
              items[j] = currentValue;
          }
      }
  }  
  

• You have a lot of magic numbers in your code of which I am unsure what they are about so I will leave them untouched. You should extract them to meaningful constants.

Implementing the mentioned points apart from the exception messages, because I don't have enough context, will lead to

private IEnumerable<byte> NextSevenA(byte skip1, byte skip2)
{
    if (skip1 == skip2)
    {
        throw new ArgumentOutOfRangeException("skip1 == skip2");
    }

    if (skip1 > 51 || skip2 > 51 || skip1 < 0 || skip2 < 0)
    {
        throw new ArgumentOutOfRangeException("51 0");
    }

    if (this.skip1.HasValue && (this.skip1 != skip1 || this.skip2 != skip2))
    {
        throw new ArgumentException("skippy");
    }

    if (skip1 == null)
    {
        this.skip1 = skip1;
        this.skip2 = skip2;
        deck50seven = GetByteRange(0, 52).Except(new byte[] { skip1, skip2 }).ToList();
    }

    nextSevenLastStart += 7;

    if (nextSevenLastStart >= 49)
    {
        nextSevenLastStart = 0;
        Shuffle(deck50seven);
    }

    return deck50seven.GetRange(nextSevenLastStart, 7);
}

---

private IEnumerable<List<byte>> NextSevenAsFiveTwo(List<byte> input)

• Using the Count() extension method on a List<T> involves a (soft)cast to an ICollection<T> so it should be faster to just use the Count property.

• Because the method is only called by requesting the enumerator you should extract the validation of the Count property to a separate method. IMO this ArgumentOutOfRangeException should be thrown at the moment you call the method.

• Instead of the most inner loop and the check for if (k != i && k != j) you can use Enumerable.Range() excepting i and j. This range can be stored in a class level (but static) variable so we can make the method static as well.

Implementing the mentioned points will look like so

private static IEnumerable<List<byte>> NextSevenAsFiveTwo(List<byte> input)
{
    if (input.Count != 7)
    {
        throw new ArgumentOutOfRangeException("input.Count != 7");
    }
    return NextSevenAsFiveTwoIterator(input);
}
private static IEnumerable<int> range = Enumerable.Range(0, 7);
private static IEnumerable<List<byte>> NextSevenAsFiveTwoIterator(List<byte> input)
{
    // 21 ways to take 2 from 7 
    // this is going to seem strange but need every combination of 2 to be the last 2

    List<byte> output = new List<byte>();
    for (int i = 0; i <= 5; i++)
    {
        for (int j = (i + 1); j <= 6; j++)
        {
            output.Clear();
            foreach (int k in range.Except(new int[] { i, j }))
            {
                output.Add(input[k]);
            }
            output.Add(input[i]);
            output.Add(input[j]);
            yield return output;
        }
    }
}