std::vector composition with self populating super-powers

Question

I have written a deliberately pretty simple class that wraps a std::vector to basically make it capable of self-populate itself with random generated numbers, simply to be able to test the result of some sorting algorithms that I'm working on.

This is only a hobby project, it is not production code :-) so I'd like to know if there are any ways to improve this design or my approach.

Besides good practices, style and design, two of my biggest doubts are:

1. populate(): If I had used std::iota() followed by std::shuffle, as explained on the comments, I believe that the code would be much smaller, simpler and would kind-of have the same result (it wouldn't allow repeated numbers, as it does now).

2. std::mt19937 and std::uniform_int_distribution: mostly, if their implementation looks... OK. Looks like the need to explicitly seed the vector is inevitable, but I don't really like it. :-( this would obviously be avoided if I used my std::iota() implementation. I feel like this breaks the class' "super-powers".

Edit: Forgot to add that, regarding number 2, I'm also a bit curious about the way I'm seeding the engine. Does it looks correct?

Following, the header file:

#ifndef algorithmicVec_H
#define algorithmicVec_H

#include <vector>
#include <random>
#include <chrono>
#include <cstdint>

/**
*  std::vector composition with pseudo-random number self-populating capabilities
*/
class algorithmicVec {
private:
  std::vector<uint32_t> vec;
  static std::mt19937 gen;
  static std::uniform_int_distribution<uint32_t> uint_dist99;

  /**
  *  Returns a random uint32_t within the range of 0..99
  */
  uint32_t getRandomNumber();

public:
  /**
  *  Seeds static std::mt19937 with the current time in milliseconds since epoch
  */
  static void seed();

  /**
  *  Populates the vector with n elements using a std::uniform_int_distribution
  *  fed with std::mt19937.
  *
  *  Using std::iota() followed by std::shuffle was considered, but not implemented
  *  mostly because random generated numbers seemed more interesting at the time,
  *  even to observe the result of the tested algorithms with possible repeated numbers.
  */
  void populate (int sz);

  /**
  *  Returns item at position pos
  */
  uint32_t at(int pos);

  /**
  *  Returns an iterator to the first item of the vector
  */
  std::vector<uint32_t>::iterator begin();

  /**
  *  Returns an iterator past-to-the-end of the vector
  */
  std::vector<uint32_t>::iterator end();
};

#endif

The implementation file:

#include "algorithmicVec.hpp"

std::mt19937 algorithmicVec::gen;
std::uniform_int_distribution<uint32_t> algorithmicVec::uint_dist99(0, 99);

void algorithmicVec::seed() {
  unsigned long seed = std::chrono::system_clock::now().time_since_epoch() /
                       std::chrono::milliseconds(1);
  gen.seed(seed);
}

void algorithmicVec::populate (int sz) {
  for (int i = 0; i < sz; ++i) {
    vec.push_back(getRandomNumber());
  }
}

uint32_t algorithmicVec::getRandomNumber() {
  return uint_dist99(gen);
}

uint32_t algorithmicVec::at(int pos) {
  return vec.at(pos);
}

std::vector<uint32_t>::iterator algorithmicVec::begin() {
  return vec.begin();
}

std::vector<uint32_t>::iterator algorithmicVec::end() {
  return vec.end();
}

A minimal test case:

#include <iostream>
#include "algorithmicVec.cpp"

int main() {
  algorithmicVec::seed();
  algorithmicVec myVec;
  myVec.populate(10); // populates myVec with 10 random numbers
  for (auto a: myVec) {
    std::cout << a << std::endl;
  }
}

Answer 1

I think it is overkill to write an own class. This gives you many issues because you have to reimplement vector's interface.

In the current state I don't see any advantage over having just a function that returns a vector that is filled randomly:

#include <vector>
#include <random>
#include <chrono>
#include <algorithm>
#include <iterator>

std::vector<uint32_t> generateRandomVector(std::size_t size) {
    static std::mt19937 gen(
            std::chrono::system_clock::now().time_since_epoch()
                    / std::chrono::milliseconds(1));
    static std::uniform_int_distribution<uint32_t> uint_dist99(0, 99);
    std::vector<uint32_t> result;
    result.reserve(size);
    std::generate_n(std::back_inserter(result), size,
            [&] {return uint_dist99(gen);});
    return result;
}

Use it like this:

#include <iostream>

int main() {
    for (auto a : generateRandomVector(20)) {
        std::cout << a << std::endl;
    }
}

This approach has the advantage of cleanly separating the functionality of the vector (which is to store data) from the functionality of the random generation (which just manipulates a vector).

Of course this interface looses some flexibility but if you need it then you should write a class that allows to create random vectors and not a vector class that happens to fill itself with random numbers.

Your Approaches

1. Using iota and shuffle does not really shorten the code of populate and somewhat "obfuscates" the thing that is going on there (so you would have to add additional comments to state what you want to achieve and why you restrict yourself to permutations).

2. The seeding of the random number generator can be done during construction which means during static initialization in the implementation file:

 

std::size_t getSeedFromCurrentTime() {
    return std::chrono::system_clock::now().time_since_epoch() /
           std::chrono::milliseconds(1);
}

std::mt19937 algorithmicVec::gen(getSeedFromCurrentTime);

About obtaining the seeding from the time I have no experience and cannot help you there.

Answer 2

Mostly style comments:

I would put the included headers in alphabetical order for better readability:

#include <chrono>
#include <cstdint>
#include <random>
#include <vector>

Always include your own headers before the Standard Library ones (this will catch errors earlier):

#include "algorithmicVec.cpp"
#include <iostream>

Unclear comment because you don't use precise interval notation. Do you mean [0, 99] (99 inclusive) or [0, 99) (99 exclusive) here?

  /**
  *  Returns a random uint32_t within the range of 0..99
  */

Comment that should be in your source control or Rationale section of the documentation:

  *  Using std::iota() followed by std::shuffle was considered, but not implemented
  *  mostly because random generated numbers seemed more interesting at the time,
  *  even to observe the result of the tested algorithms with possible repeated numbers.

Comments for at(), begin() and end() are superfluous, you could simply comment that entire section saying that they forward those parts of std::vector interface.

Furthermore, the comment for at() is wrong, because it will also throw an exception if the index is out of bounds.

Finally, as @Jamal mentions, the part of the std::vector interface that you do forward, is rather selective and incomplete (operator[], and the constant and reverse begin/end iterators come to mind).

Answer 3

• In addition to having begin() and end(), you should also have cbegin() and cend() for const iterators. These also are provided by std::vector.

• If you have at(), you should also overload operator[]. The user can choose any one, but the main difference is that at() should should throw an exception (yours doesn't do this), whereas operator[] shouldn't. This is also specified in the std::vector documentation.

• The parameter name sz for populate() doesn't seem accurate. As this is a public function, the user can call it at any time, so it's not just for object-construction. The function also adds new random numbers instead of redefining the size of the vector. A better name for this parameter could be something like numNewRandElems.

• getRandomNumber() should be const as it's not modifying any data members.

Be sure to read the std::vector documentation for more info on how it's defined.

Answer 4

• Name of the class doesn't look correct. I'd call it randomVec.

• In any case, it is a vector, so I'd recommend to inherit it.

• I don't see a value in exposing seed() method. It seems to belong to a (default) constructor. I'd also recommend to provide a constructor with an explicitly specified seed: helps a lot in debugging.

• The class should really be a template. At least parameterize the random number generator.