3
\$\begingroup\$

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);
    }
\$\endgroup\$
5
  • \$\begingroup\$ 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? \$\endgroup\$
    – Ron Beyer
    Nov 28, 2016 at 3:34
  • \$\begingroup\$ Why are you using a deck of 50 cards? A common deck is 52 cards. \$\endgroup\$
    – user33306
    Nov 28, 2016 at 3:44
  • \$\begingroup\$ @RonBeyer It does not shuffle the deck each time. Int1 and Int2 are removed from the deck. \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 8:09
  • \$\begingroup\$ @tinstaafl If you examine the question there are 52 and 2 are removed. \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 8:27
  • \$\begingroup\$ Need to add || int1 < 0 || int2 < 0 \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 9:26

2 Answers 2

2
\$\begingroup\$
 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++;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thing is "int1 == int2" would result in an index out of range exception in deckBase \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 9:25
  • \$\begingroup\$ @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. \$\endgroup\$
    – t3chb0t
    Nov 28, 2016 at 9:27
2
\$\begingroup\$
  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.

\$\endgroup\$
5
  • \$\begingroup\$ 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. \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 9:18
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ Nov 28, 2016 at 9:26
  • \$\begingroup\$ 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. \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 9:41
  • \$\begingroup\$ 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. \$\endgroup\$ Nov 28, 2016 at 9:51
  • \$\begingroup\$ 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. \$\endgroup\$
    – paparazzo
    Nov 28, 2016 at 10:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.