Small pre-C++11 pseudo-random library

Some time ago (pre-C++11) I wrote a small random number generator library as part
of a university assignment. The code has been stored on my hard drive for quite some time
and today I dug it out and decided to post it here for review. It is now obsolete, since C++11 added a very extensive random library, but still, I'd like to receive suggestions on how to improve this piece of code.

It follows a somewhat similar architecture of the standard random library and it should be
easily extensible, even though I have only provided two Engine implementations:

Random.hpp:

```
#pragma once

// ======================================================
// LCGEngine:
// ======================================================

// Implements a Linear Congruential Generator pseudo-random engine.
class LCGEngine {

public:

// Maximum value returned by NextValue().
static const int ValueMax = INT_MAX;

// Constructors:
LCGEngine();
LCGEngine(const unsigned int seed);

// Reset (seed) the pseudo-random generator engine.
void Seed(const unsigned int seed);

// Get the next value in the pseudo-random sequence.
int NextValue();

private:

// Random accumulators (LCG):
unsigned int base0;
unsigned int base1;
unsigned int base2;
};

// ======================================================
// XORShiftEngine:
// ======================================================

// Implements a XOR-Shift pseudo-random engine.
class XORShiftEngine {

public:

// Maximum value returned by NextValue().
static const int ValueMax = INT_MAX;

// Constructors:
XORShiftEngine();
XORShiftEngine(const unsigned int seed);

// Reset (seed) the pseudo-random generator engine.
void Seed(const unsigned int seed);

// Get the next value in the pseudo-random sequence.
int NextValue();

private:

unsigned int x, y, z, w;
};

// ======================================================
// Random:
// ==================================

• You use a lot of unnecessary top level cv qualifiers on parameters that don't have any effect. That doesn't only look strange to me but is also quiet some unnecessary typing effort.

• You explicitly implement copy constructors on your own whilst the compiler generated one would have exactly the same behaviour. This also means that you have to update an additional method if you'd like to extend your class.

• I suggest you to make all your non-copy constructors explicit you really don't want an unsigned to implicitly convert to Random.

• A better implementation of NextBoolean() (especially for bad engines) would be to check, whether the value is greater / less than the median. To uniformly distribute on all bits is harder to achieve than uniformly distribute on the value range.

• In general, your way to implement NextInteger(int lowerBound, int upperBound) does not completely obliterate the distribution. It does not even harm it if INT_MAX is congruent to 0 modulo (upperBound - lowerBound) or in other words is a power of two. But there is an additional problem with linear congruential generators, since their own uniform distribution and their distribution over the bits is pretty bad. For a good implementation, you have to take up to two numbers from your generator and you have to do some bit twiddling.

• Both your engines return signed integers. It looks a bit weird to me since unsigned integer are easier to work with especially if it comes to bit shifting. Just because of that you made the mistake to implement NextFloat() wrong. In the comment above you statet that its return value is in [0, 1] what would have been true if you would have used unsigned types. With signed integers, it is in [-1, 1].

• x * (1 / y) is an obfuscating way to write x / y.

• NextSymmetric() is wrong in consequence of NextFloat() being wrong.

• If one invokes NextFloat(float lowerBound, float upperBound) with a large range, the resulting values fall into categories (corresponding to their initial value returned by NextFloat() between 0 and 1) and therefore not all possible numbers in this range can be returned by this function. But at the moment neither I know a better implementation beside some platform dependend bit twiddling on the floating point numbers.

• I don't quite get the difference between NextFloat and NextUniform. Is there even any? If not, why are there two different implementations instead of NextUniform just invoking NextFloat?

• You can avoid that undefined reinterpret_cast in GenTimeSeed by using bit masks and bit shifts.

• You don't have to write inline since modern compiler know better than programmers what's beneficial to inline and what's not (they'll mostly ignore it anyways).

• Explicitly invoking the members default constructor (since this happens implicitly as well) looks weird to me but this is just an opinion.

Sorry for my bad English. Hopefully it's clear enough to understand.

StackExchange Code Review Q#63317, answer score: 3