HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Get random sets of 5 from 50

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomsetsgetfrom

Problem

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  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 = 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);
    }

Solution

private IEnumerable 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++;
}

Code Snippets

private IEnumerable<int> NextFive(int int1, int int2)
throw new IndexOutOfRangeException("int1 == int2");
throw new IndexOutOfRangeException("int1 > 51 || int2 > 51");
Random rand = new Random();
private int[] deckBase;
int nextFiveLastStart = 45;
int j = 0;
for (int i = 0; i < 52; i++)
{
    if (i == int1 || i == int2)
        continue;
    deckBase[j] = i;
    j++;
}

Context

StackExchange Code Review Q#148261, answer score: 2

Revisions (0)

No revisions yet.