Gfrktyl

A die is rolled n times. Each time, a random value has been inserted into a vector. This vector then is used to generate a sorted map (wrongly called hashmap in the code) that is later used to create an ASCII histogram.

The histogram, for example, looks like this:

    10

    #

    #

7   #

#   #

#   #     5

#   #     #

# 3 #     #

# # #     #

# # # 1   #

# # # #   #

-----------

1 2 3 4 5 6

And here's the code:

#include <iostream>

#include <vector>

#include <algorithm>

#include <string>

#include <random>

#include <map>



std::vector<int> roll(int times)

{

    std::vector<int> rand;



    while (times > 0)

    {

        std::random_device seeder;

        std::mt19937 engine(seeder());

        std::uniform_int_distribution<int> dist(1, 6);

        rand.push_back(dist(engine));



        --times;

    }



    return rand;



}



std::map<int, int> histogram_calculate(int times)

{

    std::vector<int> random_numbers = roll(times);





    std::map<int, int> cnt_hashmap;

    auto max_element = 6;



    for (int i = 1; i <= max_element; ++i)

    {

        cnt_hashmap[i] = 0;

    }



    for (auto iter = random_numbers.begin(); iter != random_numbers.end(); ++iter)

    {

        cnt_hashmap[*iter] += 1;

    }



    return cnt_hashmap;

}





std::string histogram_draw(int times)

{

    std::vector<std::string> ret_vec;

    std::map<int, int> histogram = histogram_calculate(times);



    for (int i = 1; i <= histogram.size(); ++i)

    {

        std::string to_add = "";



        if (histogram[i] > 0)

        {

            to_add = std::to_string(histogram[i]);

            std::string column = "n";

            int j = 0;



            while (j <= histogram[i])

            {

                column += "#";

                column += "n";



                ++j;

            }



            to_add += column;

        }



        to_add += "--";

        to_add += std::to_string(i);



        ret_vec.push_back(to_add);



    }



    std::string finalize = "";



    for (auto &str : ret_vec)

    {

        finalize += str;

    }



    return finalize;





}



int main() {



    std::cout << histogram_draw(10) << std::endl;

    return 0;

}

There's three functions:

• One to fill the random value vector


• One to sort out the histogram.


• One to display the histogram.

That's about it.

Hello and welcome to Code Review! I hope you have a pleasant stay.

The roll function generates a random sequence of integers. The loop body shows me that you know how to seed a pseudo-RNG with a true source of randomness but you're doing it for every iteration. You should seed the pseudo-RNG once and then use it in the loop. You know how big your rand vector will be by the time you're done with it so you should reserve memory for it so that it the memory doesn't need to be reallocated as you push integers. So your roll function should probably look like this:

std::vector<int> roll(int times)

{

    std::vector<int> rand;

    rand.reserve(times);



    std::random_device seeder;

    std::mt19937 engine(seeder());

    std::uniform_int_distribution<int> dist(1, 6);



    while (times > 0)

    {

        rand.push_back(dist(engine));

        --times;

    }



    return rand;

}

The next function, histrogram_calculate creates a histogram. The histogram is stored in a std::map<int, int> which is not really the best choice. Think about what we're storing.

1 -> 7

2 -> 3

3 -> 10

4 -> 1

5 -> 0

6 -> 5

We want to know the frequency of six numbers. Since six is a compile-time constant, we could use a std::array<int, 6> instead. Initializing an array to zero is as simple as histogram.fill(0). With a std::map<int, int> you need to set keys to zero in a loop.

auto max_element = 6;



for (int i = 1; i <= max_element; ++i)

{

    cnt_hashmap[i] = 0;

}

Using auto here is not a good idea. In fact, int is shorter and provides more information. Also, max_element should be a constant since it doesn't actually change. The max_element variable is a good start but 6 should be available to the roll function as well. You should create a global constant for 6 or perhaps use a template parameter.

// The number of sides on a die

constexpr int num_sides = 6;

histrogram_calculate iterates the vector of random numbers and counts up the frequencies.

for (auto iter = random_numbers.begin(); iter != random_numbers.end(); ++iter)

{

    cnt_hashmap[*iter] += 1;

}

Here, you're using iterators when you could be using a range-for.

for (const int num : random_numbers) {

    cnt_hashmap[num]++;

}

Wait just a minute!

histogram_calculate makes a call to roll which returns a sequence of random integers but then, it's just thrown away! Do we really need the whole sequence in order to create a histogram?

No. We can create the histogram directly from the sequence of random numbers. There's no need to create an array and then just throw it away.

I'll let you try this yourself.

histogram_draw knows how to generate a histogram. histogram_draw should take a histogram and draw it. histogram_draw should have no idea where the histogram is coming from. histrogram_draw should just draw a histogram. A histogram should be passed into the histrogram_draw function rather than the number of times to roll a die.

As @Calak pointed out in the comments, histrogram_draw doesn't actually behave as intended so this question is technically off-topic. Might I suggest that you write code to print a horizontal histogram? English text is typically written left-to-right so it is considerably easier to draw a horizontal histogram than a vertical one.

1 | ####### 7

2 | ### 3

3 | ########## 10

4 | # 1

5 | 0

6 | ##### 5

If you make improvements to your code, feel free to create a new question with your improved code. Just make sure that it functions as intended, otherwise, you risk getting flagged for being off-topic.