4
\$\begingroup\$

I have implemented a utility class to perform various random number related task. I am looking for any feedback on design / implementation. I plan to expand the class, and so would like to correct any "bad habits" and or improve the basics before I proceed.

Random.hpp

#pragma once

#include <gsl/gsl> // gsl::narrow (for safety)

#include <random>
#include <cstdint> // uintmax_t
#include <chrono> // For DefaultSeed generation
#include <iostream>
#include <type_traits>
#include <iterator> // std::distance

namespace ae
{
    namespace details
    {
        template <typename T>
        inline constexpr bool IsCharacterV
        {
            std::is_same_v<T, char>
         || std::is_same_v<T, signed char>
         || std::is_same_v<T, unsigned char>
         || std::is_same_v<T, wchar_t>
      // || std::is_same_v<T, char8_t> C++ 20
         || std::is_same_v<T, char16_t>
         || std::is_same_v<T, char32_t>
        };

        template <typename T>
        inline constexpr bool IsRealV{ std::is_floating_point_v<T> };

        template <typename T> 
        inline constexpr bool IsIntegerV{ std::is_integral_v<T> && (!IsCharacterV<T>) };

        class DefaultSeeder
        {
            using Clock = std::chrono::high_resolution_clock;

        public:
            auto& operator()() noexcept
            {
                return this->seed;
            }

        private:
            std::seed_seq seed
            {
                {
                    Clock::now().time_since_epoch().count(),
                    Clock::now().time_since_epoch().count()
                }
            };
        };
    }

    template 
    <
        typename Engine,
        typename Seeder
    >
    class BasicRandom
    {
    public:
        ////////////////////////////////////////////////////////////
        // Ranges
        ////////////////////////////////////////////////////////////

        // Integer range
        template <typename T>
        [[nodiscard]] 
        static std::enable_if_t<details::IsIntegerV<T>, T> range(T min, T max)
        {
            const std::uniform_int_distribution<T> distribution(min, max);
            return distribution(engineInstance());
        }

        // Real range
        template <typename T>
        [[nodiscard]]
        static std::enable_if_t<details::IsRealV<T>, T> range(T min, T max)
        {
            const std::uniform_real_distribution<T> distribution(min, max);
            return distribution(engineInstance());
        }

        // Call integer or real range according to common_type
        template <typename T, typename U>
        [[nodiscard]]
        static auto range(T min, U max)
        {
            using common_type = typename std::common_type_t<T, U>;
            return range(gsl::narrow<common_type>(min), gsl::narrow<common_type>(max)); // gsl::narrow will throw if the cast changed the value of its paramter
        }

        ////////////////////////////////////////////////////////////
        // Choice(s)
        ////////////////////////////////////////////////////////////

        template <typename T>
        [[nodiscard]]
        static auto choice(T first, T last) // Uses range(x, y) internally
        {
            const auto distance{ std::distance(first, last) };
            const auto rand{ range(0, distance - 1) };
            return *std::next(first, rand);
        }

        template <typename T>
        [[nodiscard]]
        static auto choice(T container) // Uses range(x, y) internally
        {
            return choice(container.begin(), container.end());
        }

        template <typename T>
        [[nodiscard]]
        static auto choices(T first, T last, std::size_t amount)
        {
            std::vector<typename std::iterator_traits<T>::value_type> results(amount);
            for (auto& val : results)
            {
                val = choice(first, last);
            }
            return results;
        }

        template <typename T>
        [[nodiscard]]
        static auto choices(T container, std::size_t amount)
        {
            std::vector<typename T::value_type> results(amount);
            for (auto& val : results)
            {
                val = choice(container.begin(), container.end());
            }
            return results;
        }

        ////////////////////////////////////////////////////////////
        // Misc
        ////////////////////////////////////////////////////////////

        template <typename T>
        [[nodiscard]]
        static auto shuffle(T first, T last)
        {
            std::shuffle(first, last, engineInstance());
        }

        template <typename T>
        [[nodiscard]]
        static auto shuffle(T& container)
        {
            std::shuffle(container.begin(), container.end(), engineInstance());
        }

        template <typename T> // Use floating point values for T..
        [[nodiscard]]
        static auto chance(T p)
        {
            const std::bernoulli_distribution distribution(p);
            return distribution(engineInstance());
        }

    private:
        [[nodiscard]] 
        static Engine& engineInstance()
        {
            static Engine engine{ Seeder{}() };
            return engine;
        }
    };

    using Random = BasicRandom<std::mt19937_64, details::DefaultSeeder>;
}

Here is some example usage, that ive also used to make sure everything works as expected:

Main.cpp

#include "Random.hpp"

#include <iostream>
#include <numeric>

int main()
{
    const auto int_range{ ae::Random::range(0, 100) };
    const auto double_range{ ae::Random::range(0.0, 50.0) };
    const auto uint_range{ ae::Random::range(5u, 100) }; // std::common_type will make the result unsigned int

    constexpr auto chance_to_roll_6{ 1.0 / 6.0 };
    constexpr auto chance_to_roll_6_twice{ chance_to_roll_6 * chance_to_roll_6 };
    const auto roll_6_with_dice{ ae::Random::chance(chance_to_roll_6) };
    const auto roll_6_with_dice_twice{ ae::Random::chance(chance_to_roll_6_twice) };

    std::array<double, 5> my_values = { 1.6, 2.5, 1.73, 3.51, 53.21 };
    const auto random_element{ ae::Random::choice(my_values) };
    const auto only_first_three_elements{ ae::Random::choice(my_values.begin(), my_values.begin() + 3) };

    const auto multiple_choices{ ae::Random::choices(my_values, 10) };
    const auto multiple_choices_only_first_2{ ae::Random::choices(my_values.begin(), my_values.begin() + 2, 10) };

    std::vector<int> my_vector(10);
    std::iota(my_vector.begin(), my_vector.end(), 0);
    ae::Random::shuffle(my_vector);

    std::cout << int_range << std::endl;
    std::cout << double_range << std::endl;
    std::cout << uint_range << std::endl;
    std::cout << roll_6_with_dice << std::endl;
    std::cout << roll_6_with_dice_twice << std::endl;
    std::cout << random_element << std::endl;
    std::cout << only_first_three_elements << std::endl;

    for (const auto& elem : multiple_choices) std::cout << elem << std::endl;
    for (const auto& elem : multiple_choices_only_first_2) std::cout << elem << std::endl;
    for (const auto& elem : my_vector) std::cout << elem << std::endl;
}
\$\endgroup\$
1
\$\begingroup\$

The design would be cleaner with per-thread engines and free functions instead of static engines based on <Engine, Seeder>:

namespace ae::random {
    using engine_type = std::mt19937_64;

    inline engine_type& engine()
    {
        thread_local eng{/* seed */};
        return eng;
    }

    // ...
}

Consider giving uniform integer distributions and uniform real distributions different names, since they are essentially different: (expressed in concepts for brevity)

template <typename T>
concept int_type = /* T is [unsigned](short|int|long|long long) */;
template <typename T>
concept real_type = /* T is float, double, long double */;

template <int_type T>
T rand_int(T min, T max)
{
    std::uniform_int_distribution dist{min, max};
    return dist(engine());
}
template <real_type T>
T rand_real(T min, T max)
{
    std::uniform_real_distribution dist{min, max};
    return dist(engine());
}

Consider constraining choice: (expressed in ranges for brevity)

template <std::random_­access_­iterator I, std::sized_sentinel_for<I> S>
iter_reference_t<I> choice(I first, S last);
template <std::random_­access_­range Rng>
range_reference_t<Rng> choice(Rng&& rng);

Similar for other functions.

The choices function can be made more flexible by providing a function for writing numbers to an (out, count) pair, or a range whose size is automatically deduced, and making choices a wrapper around it.

Also avoid std::endl when \n suffices. std::endl flushes the buffer, while \n does not. Unnecessary flushing can cause performance degradation. See std::endl vs \n.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you, would appreciate if you have time to add more. \$\endgroup\$ – Cortex Apr 19 at 14:30
  • \$\begingroup\$ @Cortex I've added more. The code is pretty good in general, and upgrading to C++20 will take some time :) \$\endgroup\$ – L. F. Apr 20 at 10:28
  • \$\begingroup\$ Excellent, I guess the less you have to say the better im doing :D You have answered alot of my post btw, appreciate your effort! \$\endgroup\$ – Cortex Apr 20 at 15:20
  • \$\begingroup\$ @Cortex You're welcome. Feel free to post more code for review in the future :) \$\endgroup\$ – L. F. Apr 21 at 8:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.