HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Random class in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomclassstackoverflow

Problem

I've very new to C++ and have been working through a tutorial and got as far as std::rand(). I was immediately horrified and rushed out to see if I could implement this as a service. This is my (pretty basic) non-cryptographically-secure implementation in the style of .Net's System.Random.

There are a number of things I'm unhappy about:

-
Anemic object. TBH there isn't much implementation here. Am I just (wrongly) trying to crowbar c++ into my .Net way of thinking? Is it a redundant layer of abstraction on top of the more powerful ` library. However, my goal is to pass around one service for random numbers (to absolutely avoid hard coding things dependencies from in my classes) and I'm not sure this can be achieved using directly.

-
Lifetime of the distributions. I'm not really sure what this should be. I've gone for the lifetime of the Random object where possible, but in the methods where I take a max-min value for each call this has not been possible. I'm pretty sure
std::mt19937 should have the same lifetime as my Random object.

-
Hard-coded uniform distributions. Preferably I would like some kind of IOC, it would be nice to pass in whether I'm using a uniform, poisson etc. I've looked inside the class and there a lot of templates, could I somehow wrap this into factory class that has methods to return int-returning-distributions, double-returning-distributions etc (would that even be a good idea?)? Given that System.Random has a hardcoded unif dist, arguably this may to too much IOC.

I think I've found an implementation that uses templates but I'm not really sure it's solving the IOC problem.

Interface: IRandom.h (Has no corresponding .cpp file)

``
#pragma once
class IRandom
{
public:
virtual int32_t Next() = 0;
virtual int32_t Next(int32_t maxValue) = 0;
virtual int32_t Next(int32_t minValue, int32_t maxValue) = 0;
virtual double_t NextDouble() = 0;
virtual double_t NextDouble(double_t minValue, double_t maxValu

Solution

Here are some comments mostly related to coding style and overall C++ practices (roughly in order of appearance):

-
Your interface is missing a virtual destructor. This is an important thing for an abstract interface and your code is actually invoking undefined behavior when trying to destroy an instance of IRandom without one. With C++11, the proper way of declaring a virtual destructor in a pure interface class is using the = default destructor idion:

virtual ~IRandom() = default;


-
Using double_t is very unusual, since double is normally the same but with less typing. Unless you really have a good reason for double_t, you should stick to the native double instead. I would say the same about using int32_t. Unless you really care about using strictly sized 32bit integers, just go for a plain ol' int.

-
By the way, when using the sized types, like int32_t and friends, from `, make sure to include the header file. You are relying on a residual include probably from , which might not be there on other compilers or even on a future version of your current one. At any rate, it is also good practice making dependencies clear.

-
When using the types from
, the correct way of referencing its members is thru the std:: namespace. They happen to also visible at the global level because on most compilers stdint.h (the C header) and cstdint (the C++ header) are implemented as wrappers to the same file. There is no requirement from the C++ Standard that this be implemented is such way, so a C++ compiler is not required to provide int32_t and friends outside the std namespace when including cstdint.

-
std::numeric_limits::[min][max]() is preferred over the INT_MAX / INT_MIN macros.

-
Prefixing in-class member access with
this-> is very rare in C++. You should only be doing that when, for instance, you need to disambiguate some shadowed member access (but even that is discouraged because shadowing members is a bit of a design flaw; better to uniquely name your variables and methods).

-
Identifiers beginning with underscore carry a few restrictions in C++ (Read this). Your use inside a class for member variables is not wrong, but might be frowned upon. Consider perhaps other styles, such as the
m_prefix or and underscore atTheEnd_. Or no distinction at all for member variables.

-
I think you probably meant to camelCase
_bytedistribution but mistyped the d in "distribution".

-
This is not the ideal way of initializing data in a constructor:

Random::Random(uint_least32_t seed)
{
    this->_randomNumberGenerator = std::mt19937(seed);
    this->_realDistribution = std::uniform_real_distribution();
    this->_bytedistribution = std::uniform_int_distribution(0, 256);
}


That way you are calling the implicit default constructor and then reinitializing the members with the assignment operator. That's an easy mistake to make when both Java and C# use that syntax, but the proper way of initializing in a C++ constructor is by calling the constructor of the member object:

Random::Random(uint_least32_t seed)
    : _randomNumberGenerator(seed)
    , _realDistribution() // <-- This one takes no arguments, so this line could even be omitted.
    , _byteDistribution(0, 256)
{ }


Notice that it is a list of comma separated constructor calls, with the first starting at the
:. This is called the constructor initialization list. C++11 also allows replacing the ( ) in the constructor calls by { }, so _randomNumberGenerator{ seed } would also be valid, if you happen to prefer that syntax.

-
While the virtual destructor in the interface definition is necessary, in your implementation class it is not. There is no reason for declaring an empty destructor but to making it a default virtual one. So you can just erase the destructor of
Random if no manual cleanup is needed.

-
#include "stdafx.h" is very Visual Studio specific and I personally dislike it since it makes your code non-portable for free. If you could get rid of it, I don't see why you shouldn't (usually involves just deleting that line and adjusting a setting on the IDE to disable precompiled headers). Edit: This point aroused some discussion in the comments section, so let me start over. I'm not advocating against the use of precompiled headers, they can have their value on large projects. My problem with stdafx.h in this particular case and other small projects is that it is going to be the only thing that prevents the code from compiling elsewhere (say with GCC or Clang). If I copy-paste this, as it stands, into a couple files, I'll have to either remove that line or create an empty stdafx.h` before I can build it, which is a nuisance, when the code could be ready to compile on other systems without any change if wasn't for this. Of course that if we are not talking about a code snippet in a website, but a source code tree distributed as a project, with a proper build sy

Code Snippets

virtual ~IRandom() = default;
Random::Random(uint_least32_t seed)
{
    this->_randomNumberGenerator = std::mt19937(seed);
    this->_realDistribution = std::uniform_real_distribution<double_t>();
    this->_bytedistribution = std::uniform_int_distribution<int32_t>(0, 256);
}
Random::Random(uint_least32_t seed)
    : _randomNumberGenerator(seed)
    , _realDistribution() // <-- This one takes no arguments, so this line could even be omitted.
    , _byteDistribution(0, 256)
{ }
auto buffer = vector<uint8_t>(100);
vector<uint8_t> buffer(100); 
//  ^            ^      ^--- (optional) constructor call
//  +--- type    |
//               +--- variable

Context

StackExchange Code Review Q#83603, answer score: 13

Revisions (0)

No revisions yet.