Program is comparing and working correctly for every array entry except array[0]

Question

I am trying to generate the numbers 1-7 in random order and add them to array. My code is supposed to generate the number, check the number generated to see if it is in the array already, and if not then add the number to the array. The only array location that is not functioning appropriately is n[0]. I have been trying hard to solve the issue but I am stuck, any help would be greatly appreciated. The last 3 times I ran the program I got:

3, 1, 5, 2, 3, 4, 7
6, 1, 3, 7, 5, 2, 6
5, 1, 7, 3, 6, 2, 5

Here is the code to generate a random number

int * randomize()
{
  static int n[7];
  int i = 0;
  int j = 0;
  int check = 0;
  int check2;

  srand((unsigned)time( NULL ));
  for( i = 0; i < 7; i++)
  {
    check = (rand() % 7);
    check += 1;

        for( j = 0; j < 7; j++)
        {
            check2 = (int) n[j];

            if(check == check2)
            {
                check = (rand() % 7);
                check += 1;
                j = 0;
            }       
        }  

      n[i] = check;
      j = 0;
    }   
return n;
}

And here is the code to call the function from main and print the array

int *r;

r = randomize();

for( i = 0; i < 7; i++){
    printf("%i\n", *(r + i));
}

Side note: it is generally better to just fill array with 1-7 and than shuffle, also for small number of entries filling out last couple elements will not take forever... Here is C# sample http://stackoverflow.com/questions/1150646/card-shuffling-in-c-sharp, but answer should be readable for anyone remotely aware of c-style syntax. — Alexei Levenkov, May 05 '15 at 00:53
Not related to your issue, but why the cast here `check2 = (int) n[j];`? — Retired Ninja, May 05 '15 at 00:59
I included the check2 = (int) n[ j ] part was something I included while trying to troubleshoot the issue. I thought maybe it just wasn't comparing things accurately because I was trying to compare incompatible types. I narrowed it down after that to the problem just being with n[ 0 ] but did not change it back yet. — Shaine, May 05 '15 at 01:16

user12205 · Answer 1 · 2015-05-05T01:02:02.337

The problem is here:

if(check == check2)
{
    check = (rand() % 7);
    check += 1;
    j = 0;
}

If a match is found, check is regenerated as a random number, but the for loop increments j by 1.

A trivial fix would be to assign -1 to j here, but it seems better to rewrite the logic instead. You can use a do-while loop with a boolean flag.

Also, note that the for(j ...) loop should really loop up to i, not 7. Otherwise you'll be comparing with values that aren't initialised.

Finally, a few points that your code can improve on, despite not being related to your current issue:

Change your

check = (rand() % 7);
check += 1;

to

check = rand() % 7 + 1;

You don't need two lines to do it, and (in my opinion) it makes your code less readable.

Also, srand() should be called at program start-up and called once only. Assuming you plan on calling randomize() multiple times, you should move the srand() call outside randomize().

And in your loop, you don't actually need a check2 variable. You can change the if condition to if(check == n[j]). If you really want to make another variable, don't call it check2 - call it something more different than your other variable. It will make your code a lot more understandable by others.

And in your output loop:

for( i = 0; i < 7; i++){
    printf("%i\n", *(r + i));
}

Although it is completely correct, I personally find using r[i] a lot more readable than *(r + i).

@Shaine You're welcome. Note that here on Stack Overflow, the question asker is expected to choose an accepted answer (by pressing the green check mark next to an answer) if the question is solved. When there are more than one answer you can choose the one that helps you the most. I recommend accepting JohnKugelman's answer since it mentions shuffling. — user12205, May 05 '15 at 01:20

John Kugelman · Accepted Answer · 2015-05-05T01:02:00.530

if(check == check2)
{
    check = (rand() % 7);
    check += 1;
    j = 0;
}

To restart the inner loop you set j to 0. When the loop goes to the next iteration j++ increments it to 1. This causes it to skip the first index. Change the assignment to:

j = -1;

If this seems a bit hacky, I agree, it is. I would suggest rearranging things a bit so you don't have to do this. The logic could be, for instance:

for (i = 0; i < 7; ++i) {
    bool dupe = false;

    do {
        n[i] = random(7) + 1;

        for (j = 0; j < i; ++j) {
            if (n[i] == n[j]) {
                dupe = true;
                break;
            }
        }
    }
    while (dupe);
}

Or, better yet, even, don't generate random numbers and then check if there are duplicates. Instead, generate all the numbers in order and then scramble the list. This will be much faster without the slowdown that will occur as the list gets fuller and fuller and it takes longer and longer to find available numbers.

Good answer, but note that OP is performing shuffle which generally done as shuffle :) — Alexei Levenkov, May 05 '15 at 00:56
Thank you for the answer, I like the idea of just scrambling the list, that should help with execution speed and solve my problem. — Shaine, May 05 '15 at 01:18

Program is comparing and working correctly for every array entry except array[0]

2 Answers2