8
\$\begingroup\$

I am currently learning C and playing around with the rand() and if/else statement. This code looks long, just curious if there is another way to make the code shorter?

The code below works fine, it's just randomize the output every time someone answers the question incorrectly.

while(input != c){
    srand(time(NULL));
    int i;
    i = rand() % 5 + 1;
    if (i == 1)
    {
        printf("No. Please try again.\n");
        scanf("%d", &input );
    }
    else if(i == 2){
        printf("Wrong. Try once more.!\n");
        scanf("%d", &input );
    }
    else if(i == 3){
        printf("Don't give up!\n");
        scanf("%d", &input );
    }
    else if(i == 4){
        printf("No. Keep trying.!\n");
        scanf("%d", &input );
    }
}

Is there any way to make it shorter, instead of using if/else over and over?

\$\endgroup\$
  • \$\begingroup\$ switch statement? \$\endgroup\$ – Funky Oct 16 '14 at 2:38
  • \$\begingroup\$ @TheFailure Mind giving me an example of switch statement? \$\endgroup\$ – user56192 Oct 16 '14 at 2:40
17
\$\begingroup\$

A standard technique is to setup an array of strings and index it with a random number:

char * answers[] = { "Wrong!", "No", "Try again"};
....
if (guess_is_wrong) {
    int i = random() % (sizeof(answers) / sizeof(answers[0]));
    printf("%s\n", answer[i]);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Yep, this is definitely the better way... \$\endgroup\$ – Funky Oct 16 '14 at 3:17
  • \$\begingroup\$ @vnp, Thank you for the quick reply. Wow, I like your solution too! \$\endgroup\$ – user56192 Oct 16 '14 at 3:57
  • \$\begingroup\$ How does that random() % (sizeof(answers) / sizeof(answers[0])) get a random value from that array? I get that the second part gets the length of the array, but the elements in here are of different size. How does that work? \$\endgroup\$ – Nzall Oct 16 '14 at 11:48
  • 1
    \$\begingroup\$ @NateKerkhofs They aren't of a different size - those are pointers to strings, not the strings themself. \$\endgroup\$ – user45891 Oct 16 '14 at 11:56
8
\$\begingroup\$

Firstly you only need to seed random once as rand() will give a new value each time it is called therefore put it outside the while loop.

You can initialize and declare a variable on the same line therefore:

int i;
i = rand() % 5 + 1;

Becomes:

int i = rand() % 5 + 1;

But seems you keep initializing i ever time the loop is run it would be better to write the initialization outside the loop.

As for the repeated if else statements they can be shortened into a switch statement if you are testing a small amount of values.

switch (*value to test*) {
    case *test equal to value*: 
        //Statements
        break; // exit the switch statement as a match has been found
    case *another value to test equal to*:
        break;
    // You can continue this for as many checks you need
    default: // If no other statements match 
}

Therefore your if else statements can be shortened to the following:

switch (i) {
    case 1:
        printf("No. Please try again.\n");
        break;
    case 2:
        printf("Wrong. Try once more.!\n");
        break;
    case 3:
        printf("Don't give up!\n");
        break;
    case 4:
        printf("No. Keep trying.!\n");
        break;
    default:
        break;
}
// Put outside as input is scanned in all conditions
scanf("%d", &input);

In conclusion my code would have been:

srand(time(NULL));
int i;

while (input != c){
    i = rand() % 5 + 1;

    switch (i) {
    case 1:
        printf("No. Please try again.\n");
        break;
    case 2:
        printf("Wrong. Try once more.!\n");
        break;
    case 3:
        printf("Don't give up!\n");
        break;
    case 4:
        printf("No. Keep trying.!\n");
        break;
    }
    scanf("%d", &input);
}

@TomHart mentions a useful thing you can do with switches which is stacking the cases, so, for example if the input 1, 2, 3 and 4 all wanted to print the same message, you could do this.

    switch (i) {
        case 1:
        case 2:
        case 3:
        case 4:
            printf("No. Please try again.\n");
            break;
    }
\$\endgroup\$
  • \$\begingroup\$ Could it be worth adding to this that cases can be stacked? \$\endgroup\$ – TMH Oct 16 '14 at 11:12
  • \$\begingroup\$ @TomHart feel free to edit the answer if you think it is needed \$\endgroup\$ – Funky Oct 16 '14 at 11:39
  • \$\begingroup\$ Okay, I'll add it in. \$\endgroup\$ – TMH Oct 16 '14 at 11:43
  • \$\begingroup\$ @TomHart Stacking cases wouldn't really help in the question (Well, not in a way I can see). In the edit describe how the stacking will help? \$\endgroup\$ – Funky Oct 16 '14 at 11:49
  • \$\begingroup\$ I agree that it doesn't really help with this particular case, but I think it's still useful information to know, as it can help to get rid of long if(a || b || c || d) style statements. \$\endgroup\$ – TMH Oct 16 '14 at 11:55
2
\$\begingroup\$

You actually have an off-by-one error in your code, but out of luck, it doesn't appear to have any ill effects other than some wasted CPU cycles.

The possible values of i after

i = rand() % 5 + 1;

are 1, 2, 3, 4, 5. What happens if i is 5?

The + 1 is superfluous. C programmers usually like to count starting from 0 — you might as well get used to it.

Once you fix that, then apply either @TheFailure's suggestion to use a switch block, or even better, use @vnp's array.


Seeding before every call to rand() defeats the purpose of the pseudo-random number generator — it reduces it to a hash function of the clock. You should only need to call srand() once at the beginning of the program.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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