11
\$\begingroup\$

Is there a more efficient algorithm to generate random numbers in C++?

(This code is working on Dev-C++, but I'm not sure if it works on a Borland compiler.)

/*
  Author: Arpit Agrawal
  Email: arpitagrawal294@gmail.com
  Description: Dice Roll Algorithm.
  Project Name: e-Roll.

*/


#include<iostream.h>
#include<conio.h>
#include<time.h>
#include<stdlib.h>
#include <stdio.h>  
#include <Windows.h>

void call();
void one();
void two();
void three();
void four();
void five();
void six();
void call();
int main()
{

    //gotoxy(30,15);
    cout<<"\n\n\n\n\t\tAuthor: Arpit Agrawal\n\t\tEmail: arpitagrawal294@gmail.com\n\t\tDescription: Dice Roll Algorithm.\n\t\tProject Name: e-Roll.\n\t\t" ; 
    cout<<"\n\n\t\tLoading. . . . . . . ";
    Sleep(3000);
    cout<<"\n\n\t\tPress r to roll or q to quit the game "<<endl;
    char ch;
    ch = getch();
    xm:
    if (ch=='r'){
    system("cls"); 
    call();  }
    else
    exit (0);
    cout<<endl<<endl<<"Press r to roll again q to quit!";
    ch = getch();
    goto xm;
    getch();
}



    void call()
    {
        srand (time(NULL));

        int n;
        n= rand();
        n = 1 + n % 6;

        switch (n)
        {
         case 1:
              one();
              break;
         case 2:
              two();
              break;
         case 3:
              three();
              break;
         case 4:
              four();
              break;
         case 5:
              five();
              break;
         case 6:
              six();
              break;
         default:
                 cout<<"NONUM";

        }       
    }  


        void one()
        {
        cout << " -----" << endl;
        cout << "|     |" << endl;
        cout << "|  O  |" << endl;
        cout << "|     |" << endl;
        cout <<  " -----" << endl;
        }
        void two()
        {
        cout << " -----" << endl;
        cout << "|    O|" << endl;
        cout << "|     |" << endl;
        cout << "|O    |" << endl;
        cout <<  " -----" << endl;
        }
        void three()
        {
        cout << " -----" << endl;
        cout << "|    O|" << endl;
        cout << "|  O  |" << endl;
        cout << "|O    |" << endl;
        cout <<  " -----" << endl;
        }
        void four()
        {
        cout << " -----" << endl;
        cout << "|O   O|" << endl;
        cout << "|     |" << endl;
        cout << "|O   O|" << endl;
        cout <<  " -----" << endl;
        }
        void five()
        {
        cout << " -----" << endl;
        cout << "|O   O|" << endl;
        cout << "|  O  |" << endl;
        cout << "|O   O|" << endl;
        cout <<  " -----" << endl;
        }
        void six()
        {
        cout << " -----" << endl;
        cout << "|O   O|" << endl;
        cout << "|O   O|" << endl;
        cout << "|O   O|" << endl;
        cout <<  " -----" << endl;
        }
\$\endgroup\$
14
\$\begingroup\$

Only call srand() once in an application:

    srand (time(NULL));

Should be just after main() starts.

This does not generate an evenly distributed the random numbers.

    n= rand();
    n = 1 + n % 6;

This is because rand() returns a number from [0,RAND_MAX) or [0,32767) which is not exactly divisible by 6. So you get:

    1:  1/5462        Notice this is one more than the others.
    2:  1/5461
    3:  1/5461
    4:  1/5461
    5:  1/5461
    6:  1/5461

Probably not an issue for a simple app but worth noting. The proper way to do this is:

int dieRoll() // 1-6 evenly distributed.
{
     static int const max = RAND_MAX/6*6;

     int r = rand();
     while(r >= max) { r = rand();}

     return r%6+1;
}

Probably best not to use goto:

xm:
if (ch=='r'){
system("cls"); 
call();  }
else
exit (0);
cout<<endl<<endl<<"Press r to roll again q to quit!";
ch = getch();
goto xm;

Prefer (a standard loop):

while (ch=='r') {
    system("cls"); 
    call();

    cout<<endl<<endl<<"Press r to roll again q to quit!";
    ch = getch();
}

Lets also compress your switch statement:

    switch (n) {
        case 1:   one();break;
        case 2:   two();break;
        case 3: three();break;
        case 4:  four();break;
        case 5:  five();break;
        case 6:   six();break;
        // We know the number will never be anything else
        // so don't need the default.
    }

Don't need to use so many std::endl.

    cout << " ----- \n"
         << "|O   O|\n"
         << "|     |\n"
         << "|O   O|\n"
         <<  " -----" << endl;

std::endl is used to flush the output. If you just want a new line use "\n".

\$\endgroup\$
11
\$\begingroup\$

Function-naming

Your function names are absolutely useless. They should all be in verb form as they perform an action, making the function's purpose clear. You have already done this with call(), though the name itself is quite unhelpful since functions are called.

Based on what they're doing, call() can be renamed to something like runGame(), and the rest of them can be renamed to something like displayDiceX() (replace X with the respective number).

main() function

There is no separation between different lines of code (based on purpose), nor is there any indentation in the conditional statements. Not only that, but you use goto, which has a great potential of causing "spaghetti code" (not a good thing). Try to keep the control flow as easy to follow as possible.

I can hardly follow the logic at a glance, but the one thing that makes sense are the outputs, and even some of those are weird. It's not technically "loading" if it's sleeping for a set period of time, although I can understand the aesthetic effect you're trying to achieve. Also, do you really need to output your email address? What does that have to do with anything in this program?

Function-indentation

I also have no idea why you've indented these entire functions towards the right. You do already have main() aligned correctly, so why not the rest? At a glance, I thought all of this was part of main(), making me think that you were missing the function definitions. You also haven't indented the code inside these "number" functions, though you have done it correctly elsewhere. Try to keep things consistent.

Overall

Overall, this looks like C code and not at all C++. It's okay to keep it simple for learning purposes, but this could use a lot of work to make it look like an ideal game implementation.

If you want to take this a step further and really make it look more like C++, consider defining your own classes, such as a Game and a Die class. The Game class will define the game rules, while the Die class will represent one die and will allow you to create dice (Die objects). The Game class will handle most of the work, and main() will just need to have a Game object that can call a public interface function for running the game. Nothing else would need to be done in main().

\$\endgroup\$
6
\$\begingroup\$

Your main() is pretty messy and very hard to read... I won't comment on the while loop since someone else already did (and I agree with him), but stylistically to make your code more readable, you want to have proper indentation:

int main()
{

    //gotoxy(30,15);
    cout<<"\n\n\n\n\t\tAuthor: Arpit Agrawal\n\t\tEmail: arpitagrawal294@gmail.com\n\t\tDescription: Dice Roll Algorithm.\n\t\tProject Name: e-Roll.\n\t\t" ; 
    cout<<"\n\n\t\tLoading. . . . . . . ";
    Sleep(3000);
    cout<<"\n\n\t\tPress r to roll or q to quit the game "<<endl;
    char ch;
    ch = getch();
    xm:
    if (ch=='r') {
        system("cls"); 
        call();
    }
    else
        exit (0);

    cout<<endl<<endl<<"Press r to roll again q to quit!";
    ch = getch();
    goto xm;
    getch();
}

I'm not sure what your objectives are, but this is what your code is doing. The last getch() will never get executed ever because of the goto xm statement and it really is a mess. The 3 lines only get executed if ch=='r' so it would be more clear to have it in that block.

Anyways, the best way to do what you want to do there is a loop - it is the clearest even though it effectively does the same thing as your goto. With a goto, you have to analyze the code to determine that you want looping behavior. When someone glances at a while, they automatically know it will loop based on the condition inside the while.

\$\endgroup\$
4
\$\begingroup\$

There's no reason to use a case statement, replace it with:

void (* draw[])() = {one, two, three, four, five, six };
draw[n]();
\$\endgroup\$
  • 3
    \$\begingroup\$ Careful with the zero-based vs. one-based arrays. Also, why not store six constant strings to be printed instead of six pointers to functions that print strings? \$\endgroup\$ – 200_success Aug 13 '14 at 21:02
0
\$\begingroup\$

While rand() is fine, like Jamal said, this is very "C" code. So if you want a more C++ way of generating random numbers, use the C++ libraries for them.

#include <random>
int getRandomInRange(int min, int max)
{
    std::random_device rd; // obtain a random number from hardware
    std::mt19937 eng(rd()); // seed the generator
    std::uniform_int_distribution<> distr(min, max); // define the range
    return distr(eng);
}

(Shamelessly 'borrowed' from this SO answer every time I want a random number, as I always forget)

I should probably note that you should put the seeding of the engine somewhere other than where you're getting the number to prevent the randomness being less than random. (Although I'm suddenly less confident about that being the case with std::mt19937, given that it's not based off time). The only reason I put it together here is so there could be a self-contained function explaining the point.

\$\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.