An ASCII histogram of a die rolled n times












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.










share|improve this question











$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


















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.










share|improve this question











$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
















6












6








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.










share|improve this question











$endgroup$




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.







c++ c++14 dice ascii-art data-visualization






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Nov 13 '18 at 17:31









rolfl

90.8k13191395




90.8k13191395










asked Nov 13 '18 at 6:12









ChubakBidpaaChubakBidpaa

12217




12217








  • 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
















  • 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










1




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




$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




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




$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




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




$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




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




$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




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






$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












1 Answer
1






active

oldest

votes


















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.






share|improve this answer











$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





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














draft saved

draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207531%2fan-ascii-histogram-of-a-die-rolled-n-times%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes









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.






share|improve this answer











$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
















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.






share|improve this answer











$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














10












10








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.






share|improve this answer











$endgroup$



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.







share|improve this answer














share|improve this answer



share|improve this answer








edited Nov 14 '18 at 5:26

























answered Nov 13 '18 at 8:01









Kerndog73Kerndog73

574315




574315








  • 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














  • 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








1




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




$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




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




$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




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$
@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$
@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




$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


















draft saved

draft discarded




















































Thanks for contributing an answer to Code Review Stack Exchange!


  • Please be sure to answer the question. Provide details and share your research!

But avoid



  • Asking for help, clarification, or responding to other answers.

  • Making statements based on opinion; back them up with references or personal experience.


Use MathJax to format equations. MathJax reference.


To learn more, see our tips on writing great answers.




draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207531%2fan-ascii-histogram-of-a-die-rolled-n-times%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Full-time equivalent

さくらももこ

13 indicted, 8 arrested in Calif. drug cartel investigation