1
\$\begingroup\$

I just want to know how efficient is my code and is there any better way to do the same program in C? Can the if statements are replaced with something else or can I merge the two loops together to get roll1 and roll2 within a single loop?

Also if there is any more tricks or things that I could have added to the program to make it more interesting. Also after reading my code, what mistake you think I am making while writing the code or what can I improve to write more efficient programs, things I should keep in mind.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <windows.h>
#include <time.h>

int main()
{
    srand(time(NULL));
    int i, roll1=0, roll2=0, NumberOfRolls, RandomNUM1[50], RandomNUM2[50];
    char nextGuess;

    puts("Welcome to the Dice Roll Game");
    puts("How many times do you want to roll a dice?");
    scanf("%d", &NumberOfRolls);

    for( i=0; i<NumberOfRolls; i++ ) {
        RandomNUM1[i] = ( rand()%6 ) + 1;
        roll1 += RandomNUM1[i];
    }

    printf("\nYou Got %d in your first roll!\n", roll1);
    Sleep(3000);
    printf("\nLet's see if you can guess the value of next roll.\n");
    printf("Will it be Higher/Lower or the same? (press H/L/S)\n");
    scanf(" %c", &nextGuess);
    nextGuess = toupper(nextGuess);

        for( i=0; i<NumberOfRolls; i++ ) {
        RandomNUM2[i] = ( rand()%6 ) + 1;
        roll2 += RandomNUM2[i];
    }

    if(nextGuess=='H'){
        if(roll1<roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1>roll2){
            printf("\nUh-Oh! Bad Luck! First roll was higher, It's %d", roll2);
        }
        else if(roll1==roll2){
            printf("\nUh-Oh! Bad Luck! Both the rolls are same, It's %d", roll2);
        }
    }

    if(nextGuess=='L'){
        if(roll1>roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1<roll2){
            printf("\nUh-Oh! Bad Luck! First roll was lower, It's %d", roll2);
        }
        else if(roll1==roll2){
            printf("\nUh-Oh! Bad Luck! Both the rolls are same, It's %d", roll2);
        }

    }

    if(nextGuess=='S'){
        if(roll1==roll2){
            printf("\nYou are such a player, you guessed it right! It's %d", roll2);
        }
        else if(roll1>roll2){
            printf("\nUh-Oh! Bad Luck! First roll was higher, It's %d", roll2);
        }
        else if(roll1<roll2){
            printf("\nUh-Oh! Bad Luck! Second roll is higher, It's %d", roll2);
        }
    }

    if( (nextGuess!='H') && (nextGuess!='L') && (nextGuess!='S') ){
            printf("\nYour input is invalid. Please enter H/L/S");
        }

    return 0;
}
\$\endgroup\$
3
\$\begingroup\$
  1. First, get rid of the arrays RandomNum1 and RandomNum2.
    You never use the data you store in them, but doing the storing might lead to out-of-bounds-accesses, as you fail to sanitize the user-input.

  2. Only assume well-formedness of input which is under your control. Test whether scanf succeeds, and whether the result you get is valid.

  3. Try to unify and simplify your conditional code.

  4. Do you know that two string-literals only separated by white-space (including newlines) are fused?

  5. Try to keep to the same style for all your names, I guess variable-names should all be lower pascal-case.

  6. Adding a single whitespace around all binary operators, and after a comma, helps readability.

\$\endgroup\$
  • \$\begingroup\$ 1. But I am using those arrays to get the sum of n pseudorandom numbers and I store it in roll1 & roll2. 2. Yes, have updated that. 3. Noted. 4. Which part are you referring to? 5. Noted 6. Noted. \$\endgroup\$ – Srijan Singh Jul 17 '17 at 18:03
  • 1
    \$\begingroup\$ 1. You only store them and read them back immediately, so trivially factored out. 4. You have consecutive calls to output strings, they can be combined. \$\endgroup\$ – Deduplicator Jul 17 '17 at 18:07
  • \$\begingroup\$ So instead of this,` for( i=0; i<NumberOfRolls; i++ ) { RandomNUM2[i] = ( rand()%6 ) + 1; roll2 += RandomNUM2[i];` I can use something like this? for( i=0; i<NumberOfRolls; i++ ) { RanNUM2 = ( rand()%6 ) + 1; roll2 += RanNUM2; \$\endgroup\$ – Srijan Singh Jul 17 '17 at 18:15
  • \$\begingroup\$ Yes, though why use a temporary at all? \$\endgroup\$ – Deduplicator Jul 17 '17 at 18:20
  • \$\begingroup\$ How do I a calculate the sum then? @dedeplicator \$\endgroup\$ – Srijan Singh Jul 17 '17 at 18:32
1
\$\begingroup\$

To add to @Deduplicator fine answer.

Replace

if(nextGuess=='H'){
  ...
if(nextGuess=='L'){
  ...
if(nextGuess=='S'){
  ...
if( (nextGuess!='H') && (nextGuess!='L') && (nextGuess!='S') ){
  ...

With

if(nextGuess=='H'){
  ...
else if(nextGuess=='L'){
  ...
else if(nextGuess=='S'){
  ...
else {
  ...

Or better

switch (nextGuess) {
  case 'H': 
    ...
    break;
  case 'L': 
    ...
    break;
  case 'S': 
    ...
    break;
  default: 
    ...
\$\endgroup\$
  • \$\begingroup\$ Thanks, buddy! btw on what factors you recommend switch cases. Is it only the readability and cleanliness or something else? \$\endgroup\$ – Srijan Singh Jul 17 '17 at 18:17
  • \$\begingroup\$ @SrijanSingh Code has a 4-way int branch - time to think switch.. Review-ability (how well can someone else discern code's intent and correctness) and maintenance (how can code be extended, improved without breaking something?) \$\endgroup\$ – chux Jul 17 '17 at 18:36

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.