6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Here's your program on Clang 4.0.0 and on GCC 4.9.0. Look at the given output. \$\endgroup\$ – Calak Nov 13 '18 at 7:09
  • 4
    \$\begingroup\$ As you seem unsure: one die, two (or more) dice. Just think of it as a strange pronunciation of "dies"... \$\endgroup\$ – Toby Speight Nov 13 '18 at 8:24
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – rolfl Nov 13 '18 at 17:31
  • 1
    \$\begingroup\$ @rolfl I think you misunderstood the reason for the edit. At revision 3, the code does not produce the expected output. So revision 3 is off-topic. At revision 5, the code functions as intended. So revision 5 is on-topic. \$\endgroup\$ – Kerndog73 Nov 14 '18 at 5:24
  • 3
    \$\begingroup\$ @Kerndog73 - the answer was given at 08:01 and revision 4 was made at 14:33 . It is revision 4 that invalidates the answer. This is not about on-topicness, it is about answer invalidation. \$\endgroup\$ – rolfl Nov 14 '18 at 11:57
10
\$\begingroup\$

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.


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.

\$\endgroup\$
  • 1
    \$\begingroup\$ I know that it is well intentioned, but you wouldn't have to post an answer. It doesn't help either him or the site. \$\endgroup\$ – Calak Nov 13 '18 at 8:27
  • 1
    \$\begingroup\$ "With a std::map<int, int> you need to set keys to zero in a loop." wrong. If operator[] doesn't find the value you're looking at, the entry is created with default constructor, so 0 in this case. \$\endgroup\$ – Calak Nov 13 '18 at 8:30
  • 12
    \$\begingroup\$ @Calak disagree. Provides several useful facts for the OP. An overly draconian reading of the faq is not helpful to the site. \$\endgroup\$ – Martin York Nov 13 '18 at 8:37
  • \$\begingroup\$ @MartinYork I don't says it's bad to try to help, I wanted to post reply too, but restricted myself because the off-topic-ness of the post. It help nor the site (what a fest, if everybody try to give useful answer to all off-topic post), nor the OP, nor the answerer. Giving credit to this kind of response if a bad thing, IMHO. \$\endgroup\$ – Calak Nov 13 '18 at 8:51
  • \$\begingroup\$ I fixed the output of the code based on this answer. And I am going to choose it as the best answer. \$\endgroup\$ – ChubakBidpaa Nov 13 '18 at 14:36

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.