Random selector

Question

Overview:

The object of the type random_selector acts as a function object and randomly choose one element from the specified sequence. Length of the sequence is inferred automatically. Also some convenience member functions are provided. It is meant to be a part of my benchmark v2 framework.

Code:

#ifndef AREA51_RANDOM_SELECTOR_HPP
#define AREA51_RANDOM_SELECTOR_HPP
#include "random_int_generator.hpp"
#include "utilities.hpp"
#include <vector>
#include <utility>
#include <iterator>
#include <stdexcept>
template <typename T,
 typename RandomNumberGenerator = shino::random_int_generator<std::size_t>>
class random_selector
{
 std::vector<T> pool;
 RandomNumberGenerator rng;
public:
 using value_type = std::add_const_t<T>;
 using reference = value_type&;
 using iterator = typename std::vector<T>::const_iterator;
 template <typename InputIt>
 random_selector(InputIt first, InputIt last):
 pool(first, last),
 rng(0, pool.size() - 1)
 {
 if (pool.size() == 0)
 {
 throw std::invalid_argument("Range cannot be empty");
 }
 }
 random_selector(std::initializer_list<T> init_list):
 pool(init_list),
 rng(0, init_list.size() - 1)
 {
 if (pool.size() == 0)
 {
 throw std::invalid_argument("Range cannot be empty");
 }
 }
 //let T and RandomNumberGenerator decide on rule of 5
 reference operator()()
 {
 return pool[rng()];
 }
 template <typename OutputIt>
 void operator()(OutputIt first, OutputIt last)
 {
 while (first != last)
 {
 *first++ = pool[rng()];
 }
 }
 //sfinae friendly reset
 //Q stands for Qualified
 template <typename QRandomNumberGenerator,
 typename = shino::enable_sfinae<QRandomNumberGenerator,
 RandomNumberGenerator>>
 void reset_generator(QRandomNumberGenerator&& next_rng)
 {
 rng = next_rng;
 }
 std::size_t data_size()
 {
 return pool.size();
 }
 iterator begin()
 {
 return pool.cbegin();
 }
 iterator end()
 {
 return pool.cend();
 }
};
#endif //AREA51_RANDOM_SELECTOR_HPP

Demo:

#include "../src/random_selector.hpp"
#include <string>
#include <iostream>
int main()
{
 random_selector<std::string> selector({"Marzhan", "David", "Jack", "Aisulu", "Darkhan", "Akbota"});
 for (const auto& name: selector)
 {
 std::cout << name << ", ";
 }
 std::cout << "\n"
 "Choosing 10 random names from pool:\n";
 for (int i = 0; i < 10; ++i)
 {
 const std::string& current_name = selector();
 std::cout << current_name << ", ";
 }
}

Output:

Marzhan, David, Jack, Aisulu, Darkhan, Akbota,

Choosing 10 random names from pool:

Darkhan, Marzhan, Jack, Darkhan, David, Darkhan, Jack, David, Marzhan, Aisulu,

Requirements:

on type T:

• Whatever std::vector<T> requires

on type RandomNumberGenerator:

• Have constructor that accepts pair of numbers, \$a\$ and \$b\$ that tells the object to generate numbers only in range \$[a;b]\$.

• operator() that outputs data implicitly convertible to std::size_t. Range must correspond to \$[a;b]\$ in the constructor call.

• Destructible

Functions:

• Empty input range on constructor call is prohibited. If the implementation will be able to detect it, the constructor will throw std::invalid_argument or its derivative. It is safe to assume that std::distance() will be mimicked. (This will give me some breathing room in implementation details).

Design decisions:

• Weird RNG: I usually use a wrapper over the generator and distribution, so I don't care about it. May be in the future I'll have different distribution object, so I want to have both tightly coupled.

• Immutable elements: I can't come up with any case when modifying elements wouldn't be an error. May be a pool of random generators could, but that sounds weird at best.

• SFINAE on reset generator: first reason is obvious. Second: I remember there was a problem with universal references, but I've long since forgotten what the problem was. It just turned into a habit.

• Few constructors: implementing other ones would impose more complex requirements on T, so I decided to delegate to std::vector<T>.

Critique request:

Feel free to comment on anything. My major concern is leaving rule of 5 to T and RandomNumberGenerator, but I couldn't implement SFINAE friendly move constructor, so left it as it is.

Necessary stuff to run the demo:

The following code is not intended to be part of the review post, but if you have any thoughts feel free to comment on those as well:

random_int_generator.hpp:

#ifndef RANDOM_ENGINE_HPP
#define RANDOM_ENGINE_HPP
#include <random>
#include <memory>
#include <type_traits>
#include <utility>
template <typename T>
inline constexpr bool is_integer_type = std::is_integral_v<T>;
namespace shino {
 template<typename IntegerType = int, typename RandomNumberEngine = std::mt19937_64>
 class random_int_generator {
 std::unique_ptr<RandomNumberEngine> engine;
 std::uniform_int_distribution<IntegerType> dist;
 public:
 template <typename ... Ts>
 random_int_generator(IntegerType first = 0,
 IntegerType last = std::numeric_limits<IntegerType>::max(),
 Ts&& ... args):
 engine(std::make_unique<RandomNumberEngine>(std::forward<Ts>(args)...)),
 dist(first, last)
 {}
 //allow move construction since it is disabled by deleted copy constructor
 random_int_generator(random_int_generator&& other) = default;
 void range(IntegerType first,
 IntegerType last = std::numeric_limits<IntegerType>::max())
 {
 dist = std::uniform_int_distribution<IntegerType>(first, last);
 }
 std::pair<IntegerType, IntegerType>
 range()
 {
 return {dist.a(), dist.b()};
 };
 template<typename OutputIt>
 void operator()(OutputIt first, OutputIt last)
 {
 while (first != last) {
 *first++ = dist(*engine);
 }
 }
 IntegerType operator()() {
 return dist(*engine);
 }
 /*
 * Providing const versions doesn't make sense because
 * it is impossible to do anything meaningful with
 * const random int generator
 * */
 RandomNumberEngine& get_engine()
 {
 return *engine;
 }
 std::uniform_int_distribution<IntegerType>&
 get_distribution()
 {
 return dist;
 }
 };
}
#endif

excerpt from utilities.hpp:

template <typename QualifiedType, typename OriginalType>
using enable_sfinae = std::enable_if_t<std::is_same_v<std::decay_t<QualifiedType>, OriginalType>>;

• Rakete1111: Just a small note: You seem to use using things from the shino namespace. We have no access to that namespace and its contents, so we cannot execute the code or use some specific things from it. Or did I miss something?
• Incomputable: @Rakete1111, yeah, sorry. It is part of my Area51 repo, and it is a mess. I'm ashamed to put links to it here, so I don't :) I'll put the code required to run it into the post.
• Incomputable: @Rakete1111, done. I believe it will also clarify the first design decision. It still won't run out of the box, but adding some namespaces and includes should make it work.
• Rakete1111: Thanks! Inline variables are only a thing since C++17, not C++14. Maybe change the relevant tags?

Answer 1

Here are some general comments:

1. random_selector doesn't have a virtual destructor, which probably means you don't need/want someone to inherit from it. It that it the case, mark it as final (if not, provide a virtual destructor):

   // ...
   class random_selector final
   // ...
   

2. You don't seem to use to modify pool. Why not mark it as const?

3. Mark functions that don't throw noexcept.

4. Some functions (like data_size, begin, end, ...) have no reason not to be called when the selector is const. Mark them as const:

   const random_selector rs;
   // std::size_t data_size();
   rs.data_size(); // illegal
   // std::size_t data_size() const noexcept
   rs.data_size(); // legal (noexcept because it doesn't throw)
   

5. Leverage the fact that std::initializer_list provides begin and end functions to implement a constructor in terms of another (which is good practice - avoids duplication):

   random_selector(std::initializer_list<T> init_list) :
    random_selector(init_list.begin(), init_list.end()) {}
   

6. I would have used std::generate instead of that hand rolled loop:

   std::generate(first, last, [&rng, &pool]() {
    return pool[rng()];
   }); 
   

7. Use std::vector::empty instead of std::vector::size() == 0.

8. Instead of constructing the generator with the range as arguments, I would pass the range when calling the generator. I would also pass the range as a constructor argument. This has 2 advantages:

   • You can nicely pass a lambda:

     random_selector selector{ { 1, 2, 3 }, [](int min, int max) { return 42; } };
     

   • You can pass a function, instead of a function object.

9. You're naming is a bit misleading: reference is a const& and iterator is const. Consider changing them to const_reference and const_iterator respectively.

• Incomputable: I think that instead of passing a lambda one can pass disguised *this.

Answer 2

static_assert() and <type_traits>could document at least some of your requirements, resulting in clearer compile-time messages.

C++17 seems to feature std::sample. Just mentioning.

Lately I am sick of Constilness, so this might be biased. More const! For example const std::vector<T> pool;.