CODEWITHSUNDEEP
carrays
Shaine
Shaine

Reputation: 37

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

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

Upvotes: 1

Views: 55

Answers (2)

user12205
user12205

Reputation: 2702

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).

Upvotes: 3

John Kugelman
John Kugelman

Reputation: 361585

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.

Upvotes: 3

Related Questions