Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
4  
I didn't know Putin could code... –  Simon André Forsberg Aug 30 at 16:50
    
@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 ;-) –  TemplateRex Aug 30 at 19:21
    
Just out of curiosity, why the downvote? –  Vladimir Putin Aug 30 at 19:33

2 Answers 2

up vote 5 down vote accepted

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) << " ";
    }
}
share|improve this answer
    
Whoops, forgot about the difference between '' and "" when copying the code over. –  Vladimir Putin Aug 30 at 16:19

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.

share|improve this answer
    
+1 for pointing out the problem with rand() % n –  OMGtechy Aug 31 at 22:52

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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