views:

82

answers:

6

Hello, for my program I need pseudo random integers with different ranges. Until now I used the rand() function but it has it's limitations.

I found the boost::random library to be a much better replacement but I didn't want to create random generators all over the place.
( I need random integers in many classes, because it's a stress test software that makes every decision pseudo-randomly ( -> a test run has to be repeatable by setting the same start seed ) ).

That's why I capsuled boost::random away in my own class.

The idea behind this is to ease the use so that it is almost as straightforward as the C++ rand() method

#include "boost/shared_ptr.hpp"
#include "boost/random.hpp"

class Random{
public:
   typedef boost::shared_ptr< Random > randomPtr;
   typedef boost::mt19937 randomGeneratorType;

   static randomPtr Get(){
      static randomPtr randomGen( new RandomGenerator() );
      return randomGen;
   }

   void SetSeed(int seed){
      randomGenerator.seed( seed );
   }

   int Random( int lowerLimit, int upperLimit ){
   boost::uniform_int<> distribution( lowerLimit, upperLimit );
   boost::variate_generator< randomGeneratorType&, boost::uniform_int<> >
   LimitedInt( randomGenerator , distribution );
   return LimitedInt();
   }

private:
   // prevent creation of more than one object of the LogManager class
   // use the Get() method to get a shared_ptr to the object
  Random():
    randomGenerator() //initialize randomGenerator with default constructor
  {}

  RandomGenerator( const RandomGenerator& orig ){};

  randomGeneratorType randomGenerator;
};

Generating a random number within a given range will now be as easy as

#include "Random.h"
  Random::Get()->SetSeed( 123123 );  // If you want to make the run repeatable
  int dice = Random::Get()->Random(1,6);

Question:
Is there anything wrong with this way of generating random numbers?
Large overhead I didn't recognize ?
Pure Evil or outdated programming technique ?

( I'm still new to c++ and want to improve my skills, and I have found that Stack overflow is the best place to get high quality advice )

A: 

I'd say this looks fine -- this way you can easily replace your random generation algo should it be needed.

Chaoz
+3  A: 

You've essentially wrapped your generator in a singleton, introducing all the problems that singletons and global variables carry. For example, you'd have difficulty getting multiple stress tests running in parallel, as your implementation isn't thread safe.

But the main problem that I see is that your wrapper isn't simpler than just using boost::random without the wrapper.

Joe Gauterin
I understood it so that I would have to write everything that is listed in my Random() method for each class where I need random numbers and additionally I would have to create a instance of boost::mt19937 (or anything else ) per class ?
zitroneneis
+1  A: 

You could probably avoid Get(). This is purely subjective, to me. I would prefer a calling mechanism like Random::Seed() and Random::Next() or Random::Next(min,max). There aren't too many function in Random so you could make them all static functions.

Here's a simple implementation. But bear in mind that this is considering you're using this in a single-threaded environment. For multi-threaded environment it's better to not have it as a singleton.

class Random
{
public:
    typedef boost::mt19937 RandomGeneratorType;

    static void Seed(int seed)
    {
        s_randGen.seed(seed);
    }

    static int NextInt(int min_val, int max_val)
    {
        boost::uniform_int<> distribution(min_val, max_val);boost::variate_generator< randomGeneratorType&, boost::uniform_int<> >
        return LimitedInt( s_randGen , distribution );;
    }
private:
    static RandomGeneratorType s_randGen;
};

Random::RandomGeneratorType Random::s_randGen;
Vite Falcon
This looks good.Thank you and +1 for code ;-)
zitroneneis
Answer with 0 benefit, using a `static` member is no better than using a local `static`: it's basically just another implementation of the very same Global.
Matthieu M.
@Matthieu: It DOES have benefit. Avoids a few function calls (like getting the pointer from boost::shared_ptr, and the static Get()). And like I said, it's a subjective issue. For you, it seems nasty, while for me it makes perfect sense.
Vite Falcon
@Vite: It mostly has drawbacks. Not using lazy initialization means potentially using an uninitialized object (Undefined Behavior) and not using a `shared_ptr` to wrap the global means potentially using an already destructed object (Undefined Behavior). For the sake of a few inlined function call ? Please read on `Initialization Order Fiasco` and `Destruction Order Fiasco`.
Matthieu M.
@Matthieu: Your comment does make sense, but in this context, I don't really see anything that boost::mt19937 relies on to be initialised before it gets.<br/>To make it clear 'Initialisation Order Fiasco' = `In short, suppose you have two static objects x and y which exist in separate source files, say x.cpp and y.cpp. Suppose further that the initialization for the y object (typically the y object's constructor) calls some method on the x object.`. To me, all u're trying to do is make a fancy comment to prove others wrong when it's not in the context.
Vite Falcon
@Matthieu: .. (contd). You're argument is good in terms for general practice, but not a drawback in this context.
Vite Falcon
@Vite: this problem appears if you use the random generator in the construction or destruction of another global object located in another translation unit. It wasn't targeted to the initialization / destruction of `s_randGen` itself.
Matthieu M.
A: 

You can also try something like create entity in a container and random shuffle them.

void GenerateRandomString(vector& container, int size_of_string, unsigned long long num_of_records, int thread_id) { srandom(time(0)); random(); for(unsigned long long int i=0; i < num_of_records; ++i) { stringstream str_stream; str_stream.clear(); str_stream << left << setfill('x') << setw(size_of_string-4); str_stream << num_of_records+i+1 << "-" << thread_id; container.push_back(str_stream.str()); } random_shuffle(container.begin(), container.end()); }

Sudhendu Sharma
Can you edit your code so it will be parsed and displayed nicely by SO? Take advantage of the markdown syntax. http://daringfireball.net/projects/markdown/syntax
Shaihi
+1  A: 

Joe Gauterin demonstrated the issue, however it didn't offered any solution :)

The problem with shared state is the absence of reentrance: ie, executing twice the same method does not provide the same result. This is particularly critical in multithreaded situations because the global state may not always change at the same point in the program, thus leading to inconsistent results from one run to another.

The solution is that each simulation should have its own "state" and then you would avoid the shared state.

This can be accomplished in a number of ways: you could still use a "global" state but make it local to a thread, for example, thus the threads would not step on each others toes.

The cleaner version, however, consists in storing this state somewhere, and the easier way is to have some kind of Context class, instantiated once per simulation, and which is an aggregate of the state of the simulation (for simulation-wide state).

With that in mind:

class Context
{
public:
  typedef boost::mt19937 RandomGeneratorType;

  void SetSeed(int seed){
     rg.seed( seed );
  }

  int Next( int lowerLimit, int upperLimit ) {
    boost::uniform_int<> distribution( lowerLimit, upperLimit );
    boost::variate_generator< randomGeneratorType&, boost::uniform_int<> >
    LimitedInt( rg, distribution );
    return LimitedInt();
  }

private:
  RandomGeneratorType rg;
};

Then, pass the Context instance around in your simulation, and you can run as many as you wish in parallel.

Matthieu M.
This makes sense. Thank you for the clarification.
zitroneneis