1
\$\begingroup\$

This is my first attempt at writing a program, so bear with me. I tried to use as many methods as I learned in about a week-long period.

#include <iostream>
#include <stdlib.h>
#include <time.h>


void generator(char tree, char water, char land)
{
    const char tileList[] = {tree, water, land};
    int tileIndex = rand() % 3;

    std::cout << tileList[tileIndex] << " ";
}


int main()
{
    srand(time(NULL));

    while(true)
    {
        generator('T', '~', '.');
    }
}

I took out comments to improve code readability. What can be improved? What needs to be changed?

\$\endgroup\$
3
  • 5
    \$\begingroup\$ I didn't know Putin could code... \$\endgroup\$ Aug 30 '14 at 16:50
  • \$\begingroup\$ @SimonAndréForsberg he has already too many distractions to be coding properly, although I'm sure he could pick it up if he wanted to ;-) \$\endgroup\$ Aug 30 '14 at 19:21
  • \$\begingroup\$ Just out of curiosity, why the downvote? \$\endgroup\$ Aug 30 '14 at 19:33
6
\$\begingroup\$

This won't compile:

void generator(char tree, char water, char land);
// ...
generator("T", "~", ".");

Because generator takes 3 chars, and you're giving it const char* instead.

Of course that's easy to fix, just make the params chars:

generator('T', '~', '.');

It would be better if the generator generated, instead of printing. It's to preserve the single responsibility principle. Like this:

char generator(char tree, char water, char land)
{
    const char tileList[] = {tree, water, land};
    int tileIndex = rand() % 3;
    return tileList[tileIndex];
}

int main()
{
    srand(time(NULL));

    while(true)
    {
        std::cout << generator('T', '~', '.') << " ";
    }
}

Of course the loop will never end... Are you ok with that?


Taking 3 chars as possible tiles doesn't seem too flexible. How about making it work with arbitrary number of tiles? Something like this:

char generator(const char * tiles, int len)
{
    int index = rand() % len;
    return tiles[index];
}

int main()
{
    srand(time(NULL));

    const char * tiles = "T~.";
    int len = strlen(tiles);

    while(true)
    {
        std::cout << generator(tiles, len) << " ";
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ Whoops, forgot about the difference between '' and "" when copying the code over. \$\endgroup\$ Aug 30 '14 at 16:19
4
\$\begingroup\$

Doing rand that way is bad.

The exact value of RAND_MAX can very but is usually 32767 (or multiples thereof).
This means rand() % 3 gives you:

  0:     1/10923   // Notice that 0 and 1
  1:     1/10923   // have a slightly higher probability.
  2:     1/10922

There are all sorts of other issues with rand. So you may want to look at the new random libraries provided with C++11.

But if you want to use rand() in the most non biased way possible.

 int getRand(int max)
 {
     int max = RAND_MAX/max*max; // Note integer division
                                 // So /max*max does not cancel out.

     int val;
     do
     {
          val = rand();
     } while(val >= max);

     return val % max;
}

At least that will give you an even distribution.

\$\endgroup\$
0

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.