# Get random sets of 5 from 50

From a deck of 50 need to get sets of 5 as random as possible and as fast as possible. The thought here is to shuffle to get 10 sets of five at a time with no collisions.

Int1 and Int2 will not change during a run. If they do other bad stuff would happen.

    public MainWindow()
{
InitializeComponent();
Stopwatch sw = new Stopwatch();
sw.Start();
for(int i = 0; i < 100000; i++ )
{
//Debug.WriteLine(string.Join(", ", NextFive(11, 12)));
int count = NextFive(11, 12).Count();
}
sw.Stop();
Debug.WriteLine(sw.ElapsedMilliseconds.ToString("N0"));
}
Random rand = new Random();
private int[] deckBase;
int nextFiveLastStart = 45;
private IEnumerable<int> NextFive(int int1, int int2)
{
if (int1 == int2)
throw new IndexOutOfRangeException("int1 == int2");
if (int1 > 51 || int2 > 51)
throw new IndexOutOfRangeException("int1 > 51 || int2 > 51");
if (deckBase == null)
{
deckBase = new int[50];
int j = 0;
for (int i = 0; i < 52; i++)
{
if (i == int1 || i == int2)
continue;
deckBase[j] = i;
j++;
}
}
if (nextFiveLastStart >= 45)
{
nextFiveLastStart = 0;
// Yates shuffle
for (int i = 49; i >= 1; i--)
{
int j = rand.Next(i + 1);
if (j != i)
{   // exchange values
int curVal = deckBase[i];
deckBase[i] = deckBase[j];
deckBase[j] = curVal;
}
}
}
else
nextFiveLastStart += 5;
return deckBase.Skip(nextFiveLastStart).Take(5);
}

• It isn't completely clear to me why you want to shuffle the deck each time? If you shuffle the deck once sufficiently randomly, just choose 5 cards and remove them from a List. That way the card isn't in the deck anymore so you can't have collisions and it was shuffled randomly once. What does int1 and int2 represent? Those aren't very descriptive names? Commented Nov 28, 2016 at 3:34
• Why are you using a deck of 50 cards? A common deck is 52 cards.
– user33306
Commented Nov 28, 2016 at 3:44
• @RonBeyer It does not shuffle the deck each time. Int1 and Int2 are removed from the deck. Commented Nov 28, 2016 at 8:09
• @tinstaafl If you examine the question there are 52 and 2 are removed. Commented Nov 28, 2016 at 8:27
• Need to add || int1 < 0 || int2 < 0 Commented Nov 28, 2016 at 9:26

 private IEnumerable<int> NextFive(int int1, int int2)


The names int1 and int2 give absolutely no clue about what they are good for. I tried to ready the code but I cannot figure it out. Looking at if (i == int1 || i == int2) I guess it means exclude but who knows.

throw new IndexOutOfRangeException("int1 == int2");


This isn't IndexOutOfRangeException but rather ArgumentException. The argumetns have invalid values but they might be within the allowed range.

throw new IndexOutOfRangeException("int1 > 51 || int2 > 51");


I agree this is the right type of the exception but I preferred it checking each value separately to give the user a hint which one of the parameters is incorrect.

Random rand = new Random();
private int[] deckBase;
int nextFiveLastStart = 45;


Inconsistent access modifier usage. private implicit, private explicit, private implicit... Pick one ;-)

int j = 0;
for (int i = 0; i < 52; i++)
{
if (i == int1 || i == int2)
continue;
deckBase[j] = i;
j++;
}


There's no need for the int j = 0; you can put it inside the for:

for (int i = 0, j = 0; i < 52; i++)
{
if (i == int1 || i == int2)
continue;
deckBase[j] = i;
j++;
}

• Thing is "int1 == int2" would result in an index out of range exception in deckBase Commented Nov 28, 2016 at 9:25
• @Paparazzi I mean the check is legitimate but the exception type is wrong. I preferred ArgumentException saying that the parameters must not be equal and actually the out-of-range test should take place first. Commented Nov 28, 2016 at 9:27
1. This isn't really production code, is it? There's an obvious test method which is testing a private method. What should be the API?

2. The chunks of five are returned as

    return deckBase.Skip(nextFiveLastStart).Take(5);


There are at least two problems with this line. Firstly, Skip is slow. You're losing the constant-time access of the array. Secondly, it's a lazy enumerable which keeps a reference to deckBase. If I call NextFive twenty times, hanging on to the return values each time, and then enumerate them as so:

var seqs = Enumerable.Range(1, 10).
Select(i => TakeFive(11, 12)).
ToArray();
seqs = seqs.Select(seq => seq.ToArray());


then I'll only have ten distinct sequences (as compared by SequenceEqual).

I can't suggest concrete changes without rewriting the entire code, but I think it does need to be completely rewritten with a public API and eager enumerations.

• Does it have to be production code to be in Code Review? It will be used as a private method. What is wrong with that? I will not be using the code as you depict. Commented Nov 28, 2016 at 9:18
• It doesn't have to be for use in production, but if it's throwaway code then why would you ask for it to be reviewed? So I think it's the default expectation that code will be held to the standard of production code. And if there are particular external factors which mean that a bug isn't exercised, that doesn't mean that it's not a bug, although it could justify not fixing it provided that the limitations on use are clearly documented. Commented Nov 28, 2016 at 9:26
• Returning ToList() increased execution time by 50%. The stated question is as fast as possible. Code is either production or throw away? Thanks for your time and input. Commented Nov 28, 2016 at 9:41
• If you mean just adding ToList to the existing code, I wouldn't expect that to improve the speed: just the correctness. But using a List<> rather than an array and calling GetRange would probably be faster. Commented Nov 28, 2016 at 9:51
• I hope you don't expect ToList() to improve speed as it adds 50% execution time. Don't agree that IEnumerable is incorrect. Again thanks for your time and input. Commented Nov 28, 2016 at 10:09