A dice game called “Greed”











up vote
7
down vote

favorite












This game is a variation of the Greed dice game. Imagine we have 5 dice. We roll, them, and jot down the results. Then, based on this chart:



Three 1's => 1000 points
Three 6's => 600 points
Three 5's => 500 points
Three 4's => 400 points
Three 3's => 300 points
Three 2's => 200 points
One 1 => 100 points
One 5 => 50 point


We calculate the score. This program exactly does that. It has two functions, one is greed() which takes a vector of 5 integers between 1 and 6 and calculates the score, and the other is greed_rand() which first generates the vector randomly, and then calculates the score.



#include <iostream>
#include <vector>
#include <random>
#include <map>

std::random_device seeder;
std::mt19937 engine(seeder());
std::uniform_int_distribution<int> dist(1, 6);

typedef std::vector<int> list_type;

int greed(list_type die_rolls)
{
int ret = 0;
std::map<int, int> cnt;

for (int i = 1; i <= 6; ++i)
{
cnt[i] = 0;
}

for (auto &d : die_rolls)
{
++cnt[d];
}

if (cnt[1] == 3)
{
ret += 1000;
}
if (cnt[1] == 2)
{
ret += 200;
}
if (cnt[1] == 1)
{
ret += 100;
}
if (cnt[1] == 4)
{
ret += 1100;
}
if (cnt[1] == 5)
{
ret += 1200;
}
if (cnt[2] == 3)
{
ret += 200;
}
if (cnt[3] == 3)
{
ret += 300;
}
if (cnt[4] == 3)
{
ret += 400;
}
if (cnt[5] == 1)
{
ret += 50;
}
if (cnt[5] == 2)
{
ret += 100;
}
if (cnt[5] == 3)
{
ret += 500;
}
if (cnt[5] == 4)
{
ret += 550;
}
if (cnt[5] == 5)
{
ret += 600;
}
if (cnt[6] == 3)
{
ret += 600;
}


return ret;

}


int greed_rand()
{
list_type rolls_rand;

for (int i = 1; i <= 5; ++i)
{
rolls_rand.push_back(dist(engine));
}

return greed(rolls_rand);
}


int main() {

list_type rolls = {1, 1, 1, 5, 5};
std::cout << greed(rolls) << std::endl;
std::cout << greed_rand() << std::endl;

return 0;
}


The output of this program is:



1100
150


Note: If you're on Windows, change the random seeder to time(NULL).










share|improve this question









New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
















  • 3




    Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
    – sbecker
    9 hours ago










  • @sbecker You're right, from now on I'll choose better function and variable names.
    – ChubakBidpaa
    8 hours ago






  • 1




    It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
    – Calak
    3 hours ago















up vote
7
down vote

favorite












This game is a variation of the Greed dice game. Imagine we have 5 dice. We roll, them, and jot down the results. Then, based on this chart:



Three 1's => 1000 points
Three 6's => 600 points
Three 5's => 500 points
Three 4's => 400 points
Three 3's => 300 points
Three 2's => 200 points
One 1 => 100 points
One 5 => 50 point


We calculate the score. This program exactly does that. It has two functions, one is greed() which takes a vector of 5 integers between 1 and 6 and calculates the score, and the other is greed_rand() which first generates the vector randomly, and then calculates the score.



#include <iostream>
#include <vector>
#include <random>
#include <map>

std::random_device seeder;
std::mt19937 engine(seeder());
std::uniform_int_distribution<int> dist(1, 6);

typedef std::vector<int> list_type;

int greed(list_type die_rolls)
{
int ret = 0;
std::map<int, int> cnt;

for (int i = 1; i <= 6; ++i)
{
cnt[i] = 0;
}

for (auto &d : die_rolls)
{
++cnt[d];
}

if (cnt[1] == 3)
{
ret += 1000;
}
if (cnt[1] == 2)
{
ret += 200;
}
if (cnt[1] == 1)
{
ret += 100;
}
if (cnt[1] == 4)
{
ret += 1100;
}
if (cnt[1] == 5)
{
ret += 1200;
}
if (cnt[2] == 3)
{
ret += 200;
}
if (cnt[3] == 3)
{
ret += 300;
}
if (cnt[4] == 3)
{
ret += 400;
}
if (cnt[5] == 1)
{
ret += 50;
}
if (cnt[5] == 2)
{
ret += 100;
}
if (cnt[5] == 3)
{
ret += 500;
}
if (cnt[5] == 4)
{
ret += 550;
}
if (cnt[5] == 5)
{
ret += 600;
}
if (cnt[6] == 3)
{
ret += 600;
}


return ret;

}


int greed_rand()
{
list_type rolls_rand;

for (int i = 1; i <= 5; ++i)
{
rolls_rand.push_back(dist(engine));
}

return greed(rolls_rand);
}


int main() {

list_type rolls = {1, 1, 1, 5, 5};
std::cout << greed(rolls) << std::endl;
std::cout << greed_rand() << std::endl;

return 0;
}


The output of this program is:



1100
150


Note: If you're on Windows, change the random seeder to time(NULL).










share|improve this question









New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.
















  • 3




    Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
    – sbecker
    9 hours ago










  • @sbecker You're right, from now on I'll choose better function and variable names.
    – ChubakBidpaa
    8 hours ago






  • 1




    It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
    – Calak
    3 hours ago













up vote
7
down vote

favorite









up vote
7
down vote

favorite











This game is a variation of the Greed dice game. Imagine we have 5 dice. We roll, them, and jot down the results. Then, based on this chart:



Three 1's => 1000 points
Three 6's => 600 points
Three 5's => 500 points
Three 4's => 400 points
Three 3's => 300 points
Three 2's => 200 points
One 1 => 100 points
One 5 => 50 point


We calculate the score. This program exactly does that. It has two functions, one is greed() which takes a vector of 5 integers between 1 and 6 and calculates the score, and the other is greed_rand() which first generates the vector randomly, and then calculates the score.



#include <iostream>
#include <vector>
#include <random>
#include <map>

std::random_device seeder;
std::mt19937 engine(seeder());
std::uniform_int_distribution<int> dist(1, 6);

typedef std::vector<int> list_type;

int greed(list_type die_rolls)
{
int ret = 0;
std::map<int, int> cnt;

for (int i = 1; i <= 6; ++i)
{
cnt[i] = 0;
}

for (auto &d : die_rolls)
{
++cnt[d];
}

if (cnt[1] == 3)
{
ret += 1000;
}
if (cnt[1] == 2)
{
ret += 200;
}
if (cnt[1] == 1)
{
ret += 100;
}
if (cnt[1] == 4)
{
ret += 1100;
}
if (cnt[1] == 5)
{
ret += 1200;
}
if (cnt[2] == 3)
{
ret += 200;
}
if (cnt[3] == 3)
{
ret += 300;
}
if (cnt[4] == 3)
{
ret += 400;
}
if (cnt[5] == 1)
{
ret += 50;
}
if (cnt[5] == 2)
{
ret += 100;
}
if (cnt[5] == 3)
{
ret += 500;
}
if (cnt[5] == 4)
{
ret += 550;
}
if (cnt[5] == 5)
{
ret += 600;
}
if (cnt[6] == 3)
{
ret += 600;
}


return ret;

}


int greed_rand()
{
list_type rolls_rand;

for (int i = 1; i <= 5; ++i)
{
rolls_rand.push_back(dist(engine));
}

return greed(rolls_rand);
}


int main() {

list_type rolls = {1, 1, 1, 5, 5};
std::cout << greed(rolls) << std::endl;
std::cout << greed_rand() << std::endl;

return 0;
}


The output of this program is:



1100
150


Note: If you're on Windows, change the random seeder to time(NULL).










share|improve this question









New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











This game is a variation of the Greed dice game. Imagine we have 5 dice. We roll, them, and jot down the results. Then, based on this chart:



Three 1's => 1000 points
Three 6's => 600 points
Three 5's => 500 points
Three 4's => 400 points
Three 3's => 300 points
Three 2's => 200 points
One 1 => 100 points
One 5 => 50 point


We calculate the score. This program exactly does that. It has two functions, one is greed() which takes a vector of 5 integers between 1 and 6 and calculates the score, and the other is greed_rand() which first generates the vector randomly, and then calculates the score.



#include <iostream>
#include <vector>
#include <random>
#include <map>

std::random_device seeder;
std::mt19937 engine(seeder());
std::uniform_int_distribution<int> dist(1, 6);

typedef std::vector<int> list_type;

int greed(list_type die_rolls)
{
int ret = 0;
std::map<int, int> cnt;

for (int i = 1; i <= 6; ++i)
{
cnt[i] = 0;
}

for (auto &d : die_rolls)
{
++cnt[d];
}

if (cnt[1] == 3)
{
ret += 1000;
}
if (cnt[1] == 2)
{
ret += 200;
}
if (cnt[1] == 1)
{
ret += 100;
}
if (cnt[1] == 4)
{
ret += 1100;
}
if (cnt[1] == 5)
{
ret += 1200;
}
if (cnt[2] == 3)
{
ret += 200;
}
if (cnt[3] == 3)
{
ret += 300;
}
if (cnt[4] == 3)
{
ret += 400;
}
if (cnt[5] == 1)
{
ret += 50;
}
if (cnt[5] == 2)
{
ret += 100;
}
if (cnt[5] == 3)
{
ret += 500;
}
if (cnt[5] == 4)
{
ret += 550;
}
if (cnt[5] == 5)
{
ret += 600;
}
if (cnt[6] == 3)
{
ret += 600;
}


return ret;

}


int greed_rand()
{
list_type rolls_rand;

for (int i = 1; i <= 5; ++i)
{
rolls_rand.push_back(dist(engine));
}

return greed(rolls_rand);
}


int main() {

list_type rolls = {1, 1, 1, 5, 5};
std::cout << greed(rolls) << std::endl;
std::cout << greed_rand() << std::endl;

return 0;
}


The output of this program is:



1100
150


Note: If you're on Windows, change the random seeder to time(NULL).







c++ game random c++14 dice






share|improve this question









New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 2 hours ago









Toby Speight

21.8k536107




21.8k536107






New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 17 hours ago









ChubakBidpaa

825




825




New contributor




ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






ChubakBidpaa is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.








  • 3




    Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
    – sbecker
    9 hours ago










  • @sbecker You're right, from now on I'll choose better function and variable names.
    – ChubakBidpaa
    8 hours ago






  • 1




    It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
    – Calak
    3 hours ago














  • 3




    Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
    – sbecker
    9 hours ago










  • @sbecker You're right, from now on I'll choose better function and variable names.
    – ChubakBidpaa
    8 hours ago






  • 1




    It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
    – Calak
    3 hours ago








3




3




Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
– sbecker
9 hours ago




Not enough to warrant an answer but I'm surprised the very good answers we have don't mentioned the function names. Though the game is called greed, wouldn't calculate_score and roll_dice be way more explanatory as function names?
– sbecker
9 hours ago












@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
8 hours ago




@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
8 hours ago




1




1




It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
– Calak
3 hours ago




It's a bug or a feature? If the we got five 6, which score you expect? 0 or 600? Actually you get 0 but I think 600 seems more logic.
– Calak
3 hours ago










5 Answers
5






active

oldest

votes

















up vote
12
down vote













Style

You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.



Globals

The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.



Counting Logic

Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice. An improvement would be to check for the larger combinations first and just continue scoring as long as possible. For example:



for(; count[1] >= 3; count[1] -= 3) 
{
score += 1000;
}
for(; count[1] > 0; --count[1])
{
score += 100;
}


This also matches the score chart more clearly.



Output
You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct. It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:



std::vector<int> roll_dice(int count, std::mt19937 &engine)


Use of Templates (Optional)

While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:



template<typename Iterator>
int greed(Iterator begin, Iterator end)
{
...
for (auto it = begin; it != end; ++it)
{
++cnt[*it];
}





share|improve this answer























  • Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
    – ChubakBidpaa
    16 hours ago


















up vote
5
down vote













Nice separation of functionality, well done!



There are some important details that the other review doesn’t address:



int greed(list_type die_rolls)


Here you are taking a std::vector by value. This means it will ve copied. It’s a small array, it probably doesn’t matter here at all, but you should get used to passing larger objects and objects that own stuff (a vector owns a piece of memory in the heap) by reference. Appropriate would be:



int greed(list_type const& die_rolls)


Next, you use a std::map<int, int> cnt to count die rolls. But you only index it using values 1-6. You should use a std::vector here. You’d waste the first element (index 0), but indexing into a vector is much faster than indexing into a map, and it takes less memory to boot. Or, since you know the size at compile time, you could use a std::array<int,7> instead. This is a fixed-sized array that lives entirely on the stack, it doesn’t allocate heap memory.



Next you do



for (int i = 1; i <= 6; ++i)
{
cnt[i] = 0;
}


Since we’re using a std::vector or std::array now, you can use std::fill:



std::fill(cnt.start(), cnt.end(), 0);


std::array even has a fill member function:



cnt.fill(0);


However, it is even easier to rely on value initialization:



std::array<int,7> cnt{};


This value-initializes each element of cnt to int{}, meaning each element will have a value of 0.






share|improve this answer























  • I didn't know about fill(). Thank you.
    – ChubakBidpaa
    14 hours ago










  • @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
    – Calak
    11 hours ago










  • @Calak: Good point! Answer updated.
    – Cris Luengo
    11 hours ago


















up vote
2
down vote













Separation



Good attempt on separation of concerns, but I think you can go further.




  • You can wrap the logic to generate a random integer in a function generate_random_int (int min, int max); where the content might looks like the example provided by Bjarne Stroustrup (look at rand_int) and so, no more globals.

  • You can wrap the logic about filling a container with random numbers in a function template<class ForwardIt> void fill_random(ForwardIt begin, ForwardIt end) (which internally can rely on std::generate and your generate_random_int).


Be explicit



Try to be explicit about "how" and "what"





  • How: Use self-explanary methods and algorithms.


  • What: Uses names that make sense


Parameters type



Try to pass types that are not cheap to copy, by const& (unless you need "destructive" work on it).



Map initialization



You don't need to initialize the map with 0's because when you try accessing to a key that doesn't exist, it is default constructed (so with 0's here).



Choose right types



As stated in other answers since your map's keys is a range of integers, you should use a std::vector or even since you know it size at compile time, std::array that you automatically fill with 0's when you default construct: std::array <int, 7> counts{};. (If you don't want to waste space of the elem at index 0, you have to do some computation later).



Occurrences counting



When you write:



if (some_var == 3) { do_something(); }             //A
if (some_var == 2) { do_something_else(); } //B


If A is true, B can never be true. Instead of re-checking when it's useless, simply use else if :



if (some_var == 3) { do_something(); } 
else if (some_var == 3) { do_something_else(); }


But...



... Instead of testing several times for each dice side possible count, you can reduce branchements checking all "combo" first:



if (counts[1] >= 3) {
result += 1000;
counts[1] -= 3; // here we decrement to remove the wombo combo from the count
}
else if (counts[2] >= 3) {
//...
}
// ...
if (counts[1] > 1) {
result += 100 * counts[1];
}
// ...


Or even, automatically compute combo count



// superbonus
if (counts[1] >= 3) {
result += 1000;
counts [1] -= 3;
}
// combo bonus
else {
for (int index = 2; index < 7; ++index) {
if (counts[index] >= 3) {
result += index * 100;
counts[index] -= 3;
break;
}
}
}
// ...
if (counts[1] > 1) {
result += counts[1] * 100;
}
// ...


Or, maybe more explicit:



// combo bonus
for (int index = 1; index < 7; ++index) {
if (counts[index] >= 3) {
result += index * 100;
counts[index] -= 3;
break;
}
}
if (result == 100) {
result *= 10; // superbonus
}
if (counts[1] > 1) {
result += counts[1] * 100;
}
// ...


Or using the @Snowhawk method, more imperative (and surely more efficient) but less flexible if you want to change your algorithm later.



User-friendly output



Instead of just printing the output, add some information to the user.
It's look nicer if he get as output:




Roll the dice! We got 1, 1, 5, 1, 5, for total of 1100 points




What's next?



Maybe a good challenge is to try implementing the full game based on complete rules? (this or this)






share|improve this answer






























    up vote
    1
    down vote













    std::random_device seeder;
    std::mt19937 engine(seeder());


    While this is fine for small hobby projects or testing, you'll likely want to properly seed std::mt19937. Let's break this down into explicit steps to understand what is going on.



    std::random_device rdev;     


    std::random_device asks the OS for system-provided randomness. It can be, and sometimes is, implemented as a simple PRNG with a fixed seed, meaning you could produce the same sequence on every run. Arguably worse than using time(NULL) as a source for entropy.



    auto random_seed{rdev()};


    Invoking the random device object returns an unsigned int. This is normally 4 bytes, but it could be 2.



    std::seed_seq seeder{random_seed};


    A seed sequence is created using that one 2/4-byte value.



    std::mt19937 engine(seeder);


    You are now attempting to initialize the internal 624 32-bit integer state of the Mersenne Twister with that one value. This leads to issues related to




    • predictability - searching for the seed is simple as there are only 2^32 possibilities.

    • bias - values like 7 and 13 will never be generated. Two different seeds produce 0, 12 different seeds produce 1226181350.


    If you are interested the perils/pitfalls of random bit generation and std::seed_seq, read through the comments here.





     typedef std::vector<int> list_type;


    If you are expecting a fixed length container, consider using std::array over std::vector.





        std::map<int, int> cnt;


    std::map is overkill for counting a contiguous range of values. std::unordered_map is better. An array-based counting sort would be best.



        for (int i = 1; i <= 6; ++i)
    {
    cnt[i] = 0;
    }

    for (auto &d : die_rolls)
    {
    ++cnt[d];
    }


    std::map and std::unordered_map will default construct a value into the container if the key doesn't exist. You can skip the value initialization for these containers.





        if (cnt[1] == 3) { ret += 1000; }
    /* ... */


    You can do some strength reduction here by reorganizing the related comparisons and converting some of the branches into integer multiplications. Since you limit the number of dice to five, you can immediately stop checking the remaining triples once the first triple has been found.



        if (cnt[1] >= 3) { score += 1000; cnt[1] -= 3; }
    else if (cnt[2] >= 3) { score += 200; }
    else if (cnt[3] >= 3) { score += 300; }
    else if (cnt[4] >= 3) { score += 400; }
    else if (cnt[5] >= 3) { score += 500; cnt[5] -= 3; }
    else if (cnt[6] >= 3) { score += 600; }

    score += cnt[1] * 100 + cnt[5] * 50;





    share|improve this answer























    • Very interesting explanation about randomization!
      – Calak
      5 hours ago






    • 1




      You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
      – Calak
      4 hours ago


















    up vote
    0
    down vote













    You can easily decouple the scoring-algorithm from the container used to score dice-rolls by using templates. Not that you actually need a container, if you have the right iterators.



    Anyway, why aren't you prepared to deal with more dice being thrown?



    Also, consider making it data-driven:



    template <class InputIt, class Sentinel = InputIt>
    auto score(InputIt first, Sentinel last) {
    constexpr auto cap = 16;
    constexpr unsigned goals[3] = {
    {1, 3, 1000},
    {6, 3, 600},
    {5, 3, 500},
    {4, 3, 400},
    {3, 3, 300},
    {2, 3, 200},
    {1, 1, 100},
    {5, 1, 50},
    };
    unsigned char dice[cap] = {};
    for (; first != last; ++first)
    ++dice[(unsigned)*first % cap];
    auto result = 0ULL;
    for (auto [which, count, reward] : goals) {
    auto& x = dice[which % cap];
    result += x / count * reward;
    x %= count;
    }
    return result;
    }


    Global mutable state is best avoided. Why is the random-generator and related things global?



    For some reason, you have extra-newlines surrounding the return of your scoring-function. Maybe you should automate indentation?



    main() is your only function where the opening brace doesn't have its own line. While there is Nothing wrong with that, try for consistency.



    return 0; is implicit for main().





    share





















      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',
      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
      });


      }
      });






      ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.










       

      draft saved


      draft discarded


















      StackExchange.ready(
      function () {
      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207689%2fa-dice-game-called-greed%23new-answer', 'question_page');
      }
      );

      Post as a guest















      Required, but never shown

























      5 Answers
      5






      active

      oldest

      votes








      5 Answers
      5






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes








      up vote
      12
      down vote













      Style

      You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.



      Globals

      The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.



      Counting Logic

      Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice. An improvement would be to check for the larger combinations first and just continue scoring as long as possible. For example:



      for(; count[1] >= 3; count[1] -= 3) 
      {
      score += 1000;
      }
      for(; count[1] > 0; --count[1])
      {
      score += 100;
      }


      This also matches the score chart more clearly.



      Output
      You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct. It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:



      std::vector<int> roll_dice(int count, std::mt19937 &engine)


      Use of Templates (Optional)

      While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:



      template<typename Iterator>
      int greed(Iterator begin, Iterator end)
      {
      ...
      for (auto it = begin; it != end; ++it)
      {
      ++cnt[*it];
      }





      share|improve this answer























      • Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
        – ChubakBidpaa
        16 hours ago















      up vote
      12
      down vote













      Style

      You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.



      Globals

      The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.



      Counting Logic

      Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice. An improvement would be to check for the larger combinations first and just continue scoring as long as possible. For example:



      for(; count[1] >= 3; count[1] -= 3) 
      {
      score += 1000;
      }
      for(; count[1] > 0; --count[1])
      {
      score += 100;
      }


      This also matches the score chart more clearly.



      Output
      You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct. It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:



      std::vector<int> roll_dice(int count, std::mt19937 &engine)


      Use of Templates (Optional)

      While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:



      template<typename Iterator>
      int greed(Iterator begin, Iterator end)
      {
      ...
      for (auto it = begin; it != end; ++it)
      {
      ++cnt[*it];
      }





      share|improve this answer























      • Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
        – ChubakBidpaa
        16 hours ago













      up vote
      12
      down vote










      up vote
      12
      down vote









      Style

      You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.



      Globals

      The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.



      Counting Logic

      Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice. An improvement would be to check for the larger combinations first and just continue scoring as long as possible. For example:



      for(; count[1] >= 3; count[1] -= 3) 
      {
      score += 1000;
      }
      for(; count[1] > 0; --count[1])
      {
      score += 100;
      }


      This also matches the score chart more clearly.



      Output
      You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct. It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:



      std::vector<int> roll_dice(int count, std::mt19937 &engine)


      Use of Templates (Optional)

      While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:



      template<typename Iterator>
      int greed(Iterator begin, Iterator end)
      {
      ...
      for (auto it = begin; it != end; ++it)
      {
      ++cnt[*it];
      }





      share|improve this answer














      Style

      You're not being charged by the character; there's no need to abbreviate "count", or "ret" (which I would call "score" instead). Also, main has inconsistent brackets with the rest of the program. Other than that, it looks good.



      Globals

      The globals are only being used by greed_rand, and would be better created within main and passed as parameters to greed_rand.



      Counting Logic

      Currently you're checking each possible quantity of '1's (and other numbers) separately, and not in order either, which makes it harder to ensure you're not missing a case and will fail if you change the number of total dice. An improvement would be to check for the larger combinations first and just continue scoring as long as possible. For example:



      for(; count[1] >= 3; count[1] -= 3) 
      {
      score += 1000;
      }
      for(; count[1] > 0; --count[1])
      {
      score += 100;
      }


      This also matches the score chart more clearly.



      Output
      You're outputting the end result of greed_rand, but not what list generated that result - which means you can't really tell if it was correct. It would probably be better to replace greed_rand with a method that makes a random list, and pass that to greed yourself, so you can also output it:



      std::vector<int> roll_dice(int count, std::mt19937 &engine)


      Use of Templates (Optional)

      While not required for this usage, greed would be a good candidate for taking a pair of generic iterators instead of requiring a std::vector:



      template<typename Iterator>
      int greed(Iterator begin, Iterator end)
      {
      ...
      for (auto it = begin; it != end; ++it)
      {
      ++cnt[*it];
      }






      share|improve this answer














      share|improve this answer



      share|improve this answer








      edited 16 hours ago

























      answered 16 hours ago









      Errorsatz

      4587




      4587












      • Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
        – ChubakBidpaa
        16 hours ago


















      • Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
        – ChubakBidpaa
        16 hours ago
















      Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
      – ChubakBidpaa
      16 hours ago




      Thank you. I don't know what templates are yet, but once I learn about them (in a few hours) I will make use of them.
      – ChubakBidpaa
      16 hours ago












      up vote
      5
      down vote













      Nice separation of functionality, well done!



      There are some important details that the other review doesn’t address:



      int greed(list_type die_rolls)


      Here you are taking a std::vector by value. This means it will ve copied. It’s a small array, it probably doesn’t matter here at all, but you should get used to passing larger objects and objects that own stuff (a vector owns a piece of memory in the heap) by reference. Appropriate would be:



      int greed(list_type const& die_rolls)


      Next, you use a std::map<int, int> cnt to count die rolls. But you only index it using values 1-6. You should use a std::vector here. You’d waste the first element (index 0), but indexing into a vector is much faster than indexing into a map, and it takes less memory to boot. Or, since you know the size at compile time, you could use a std::array<int,7> instead. This is a fixed-sized array that lives entirely on the stack, it doesn’t allocate heap memory.



      Next you do



      for (int i = 1; i <= 6; ++i)
      {
      cnt[i] = 0;
      }


      Since we’re using a std::vector or std::array now, you can use std::fill:



      std::fill(cnt.start(), cnt.end(), 0);


      std::array even has a fill member function:



      cnt.fill(0);


      However, it is even easier to rely on value initialization:



      std::array<int,7> cnt{};


      This value-initializes each element of cnt to int{}, meaning each element will have a value of 0.






      share|improve this answer























      • I didn't know about fill(). Thank you.
        – ChubakBidpaa
        14 hours ago










      • @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
        – Calak
        11 hours ago










      • @Calak: Good point! Answer updated.
        – Cris Luengo
        11 hours ago















      up vote
      5
      down vote













      Nice separation of functionality, well done!



      There are some important details that the other review doesn’t address:



      int greed(list_type die_rolls)


      Here you are taking a std::vector by value. This means it will ve copied. It’s a small array, it probably doesn’t matter here at all, but you should get used to passing larger objects and objects that own stuff (a vector owns a piece of memory in the heap) by reference. Appropriate would be:



      int greed(list_type const& die_rolls)


      Next, you use a std::map<int, int> cnt to count die rolls. But you only index it using values 1-6. You should use a std::vector here. You’d waste the first element (index 0), but indexing into a vector is much faster than indexing into a map, and it takes less memory to boot. Or, since you know the size at compile time, you could use a std::array<int,7> instead. This is a fixed-sized array that lives entirely on the stack, it doesn’t allocate heap memory.



      Next you do



      for (int i = 1; i <= 6; ++i)
      {
      cnt[i] = 0;
      }


      Since we’re using a std::vector or std::array now, you can use std::fill:



      std::fill(cnt.start(), cnt.end(), 0);


      std::array even has a fill member function:



      cnt.fill(0);


      However, it is even easier to rely on value initialization:



      std::array<int,7> cnt{};


      This value-initializes each element of cnt to int{}, meaning each element will have a value of 0.






      share|improve this answer























      • I didn't know about fill(). Thank you.
        – ChubakBidpaa
        14 hours ago










      • @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
        – Calak
        11 hours ago










      • @Calak: Good point! Answer updated.
        – Cris Luengo
        11 hours ago













      up vote
      5
      down vote










      up vote
      5
      down vote









      Nice separation of functionality, well done!



      There are some important details that the other review doesn’t address:



      int greed(list_type die_rolls)


      Here you are taking a std::vector by value. This means it will ve copied. It’s a small array, it probably doesn’t matter here at all, but you should get used to passing larger objects and objects that own stuff (a vector owns a piece of memory in the heap) by reference. Appropriate would be:



      int greed(list_type const& die_rolls)


      Next, you use a std::map<int, int> cnt to count die rolls. But you only index it using values 1-6. You should use a std::vector here. You’d waste the first element (index 0), but indexing into a vector is much faster than indexing into a map, and it takes less memory to boot. Or, since you know the size at compile time, you could use a std::array<int,7> instead. This is a fixed-sized array that lives entirely on the stack, it doesn’t allocate heap memory.



      Next you do



      for (int i = 1; i <= 6; ++i)
      {
      cnt[i] = 0;
      }


      Since we’re using a std::vector or std::array now, you can use std::fill:



      std::fill(cnt.start(), cnt.end(), 0);


      std::array even has a fill member function:



      cnt.fill(0);


      However, it is even easier to rely on value initialization:



      std::array<int,7> cnt{};


      This value-initializes each element of cnt to int{}, meaning each element will have a value of 0.






      share|improve this answer














      Nice separation of functionality, well done!



      There are some important details that the other review doesn’t address:



      int greed(list_type die_rolls)


      Here you are taking a std::vector by value. This means it will ve copied. It’s a small array, it probably doesn’t matter here at all, but you should get used to passing larger objects and objects that own stuff (a vector owns a piece of memory in the heap) by reference. Appropriate would be:



      int greed(list_type const& die_rolls)


      Next, you use a std::map<int, int> cnt to count die rolls. But you only index it using values 1-6. You should use a std::vector here. You’d waste the first element (index 0), but indexing into a vector is much faster than indexing into a map, and it takes less memory to boot. Or, since you know the size at compile time, you could use a std::array<int,7> instead. This is a fixed-sized array that lives entirely on the stack, it doesn’t allocate heap memory.



      Next you do



      for (int i = 1; i <= 6; ++i)
      {
      cnt[i] = 0;
      }


      Since we’re using a std::vector or std::array now, you can use std::fill:



      std::fill(cnt.start(), cnt.end(), 0);


      std::array even has a fill member function:



      cnt.fill(0);


      However, it is even easier to rely on value initialization:



      std::array<int,7> cnt{};


      This value-initializes each element of cnt to int{}, meaning each element will have a value of 0.







      share|improve this answer














      share|improve this answer



      share|improve this answer








      edited 11 hours ago

























      answered 15 hours ago









      Cris Luengo

      2,349319




      2,349319












      • I didn't know about fill(). Thank you.
        – ChubakBidpaa
        14 hours ago










      • @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
        – Calak
        11 hours ago










      • @Calak: Good point! Answer updated.
        – Cris Luengo
        11 hours ago


















      • I didn't know about fill(). Thank you.
        – ChubakBidpaa
        14 hours ago










      • @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
        – Calak
        11 hours ago










      • @Calak: Good point! Answer updated.
        – Cris Luengo
        11 hours ago
















      I didn't know about fill(). Thank you.
      – ChubakBidpaa
      14 hours ago




      I didn't know about fill(). Thank you.
      – ChubakBidpaa
      14 hours ago












      @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
      – Calak
      11 hours ago




      @CrisLuengo disagree. Why using std::fill (...) when you can just default construct? (std::array <int, 7> counts{};)
      – Calak
      11 hours ago












      @Calak: Good point! Answer updated.
      – Cris Luengo
      11 hours ago




      @Calak: Good point! Answer updated.
      – Cris Luengo
      11 hours ago










      up vote
      2
      down vote













      Separation



      Good attempt on separation of concerns, but I think you can go further.




      • You can wrap the logic to generate a random integer in a function generate_random_int (int min, int max); where the content might looks like the example provided by Bjarne Stroustrup (look at rand_int) and so, no more globals.

      • You can wrap the logic about filling a container with random numbers in a function template<class ForwardIt> void fill_random(ForwardIt begin, ForwardIt end) (which internally can rely on std::generate and your generate_random_int).


      Be explicit



      Try to be explicit about "how" and "what"





      • How: Use self-explanary methods and algorithms.


      • What: Uses names that make sense


      Parameters type



      Try to pass types that are not cheap to copy, by const& (unless you need "destructive" work on it).



      Map initialization



      You don't need to initialize the map with 0's because when you try accessing to a key that doesn't exist, it is default constructed (so with 0's here).



      Choose right types



      As stated in other answers since your map's keys is a range of integers, you should use a std::vector or even since you know it size at compile time, std::array that you automatically fill with 0's when you default construct: std::array <int, 7> counts{};. (If you don't want to waste space of the elem at index 0, you have to do some computation later).



      Occurrences counting



      When you write:



      if (some_var == 3) { do_something(); }             //A
      if (some_var == 2) { do_something_else(); } //B


      If A is true, B can never be true. Instead of re-checking when it's useless, simply use else if :



      if (some_var == 3) { do_something(); } 
      else if (some_var == 3) { do_something_else(); }


      But...



      ... Instead of testing several times for each dice side possible count, you can reduce branchements checking all "combo" first:



      if (counts[1] >= 3) {
      result += 1000;
      counts[1] -= 3; // here we decrement to remove the wombo combo from the count
      }
      else if (counts[2] >= 3) {
      //...
      }
      // ...
      if (counts[1] > 1) {
      result += 100 * counts[1];
      }
      // ...


      Or even, automatically compute combo count



      // superbonus
      if (counts[1] >= 3) {
      result += 1000;
      counts [1] -= 3;
      }
      // combo bonus
      else {
      for (int index = 2; index < 7; ++index) {
      if (counts[index] >= 3) {
      result += index * 100;
      counts[index] -= 3;
      break;
      }
      }
      }
      // ...
      if (counts[1] > 1) {
      result += counts[1] * 100;
      }
      // ...


      Or, maybe more explicit:



      // combo bonus
      for (int index = 1; index < 7; ++index) {
      if (counts[index] >= 3) {
      result += index * 100;
      counts[index] -= 3;
      break;
      }
      }
      if (result == 100) {
      result *= 10; // superbonus
      }
      if (counts[1] > 1) {
      result += counts[1] * 100;
      }
      // ...


      Or using the @Snowhawk method, more imperative (and surely more efficient) but less flexible if you want to change your algorithm later.



      User-friendly output



      Instead of just printing the output, add some information to the user.
      It's look nicer if he get as output:




      Roll the dice! We got 1, 1, 5, 1, 5, for total of 1100 points




      What's next?



      Maybe a good challenge is to try implementing the full game based on complete rules? (this or this)






      share|improve this answer



























        up vote
        2
        down vote













        Separation



        Good attempt on separation of concerns, but I think you can go further.




        • You can wrap the logic to generate a random integer in a function generate_random_int (int min, int max); where the content might looks like the example provided by Bjarne Stroustrup (look at rand_int) and so, no more globals.

        • You can wrap the logic about filling a container with random numbers in a function template<class ForwardIt> void fill_random(ForwardIt begin, ForwardIt end) (which internally can rely on std::generate and your generate_random_int).


        Be explicit



        Try to be explicit about "how" and "what"





        • How: Use self-explanary methods and algorithms.


        • What: Uses names that make sense


        Parameters type



        Try to pass types that are not cheap to copy, by const& (unless you need "destructive" work on it).



        Map initialization



        You don't need to initialize the map with 0's because when you try accessing to a key that doesn't exist, it is default constructed (so with 0's here).



        Choose right types



        As stated in other answers since your map's keys is a range of integers, you should use a std::vector or even since you know it size at compile time, std::array that you automatically fill with 0's when you default construct: std::array <int, 7> counts{};. (If you don't want to waste space of the elem at index 0, you have to do some computation later).



        Occurrences counting



        When you write:



        if (some_var == 3) { do_something(); }             //A
        if (some_var == 2) { do_something_else(); } //B


        If A is true, B can never be true. Instead of re-checking when it's useless, simply use else if :



        if (some_var == 3) { do_something(); } 
        else if (some_var == 3) { do_something_else(); }


        But...



        ... Instead of testing several times for each dice side possible count, you can reduce branchements checking all "combo" first:



        if (counts[1] >= 3) {
        result += 1000;
        counts[1] -= 3; // here we decrement to remove the wombo combo from the count
        }
        else if (counts[2] >= 3) {
        //...
        }
        // ...
        if (counts[1] > 1) {
        result += 100 * counts[1];
        }
        // ...


        Or even, automatically compute combo count



        // superbonus
        if (counts[1] >= 3) {
        result += 1000;
        counts [1] -= 3;
        }
        // combo bonus
        else {
        for (int index = 2; index < 7; ++index) {
        if (counts[index] >= 3) {
        result += index * 100;
        counts[index] -= 3;
        break;
        }
        }
        }
        // ...
        if (counts[1] > 1) {
        result += counts[1] * 100;
        }
        // ...


        Or, maybe more explicit:



        // combo bonus
        for (int index = 1; index < 7; ++index) {
        if (counts[index] >= 3) {
        result += index * 100;
        counts[index] -= 3;
        break;
        }
        }
        if (result == 100) {
        result *= 10; // superbonus
        }
        if (counts[1] > 1) {
        result += counts[1] * 100;
        }
        // ...


        Or using the @Snowhawk method, more imperative (and surely more efficient) but less flexible if you want to change your algorithm later.



        User-friendly output



        Instead of just printing the output, add some information to the user.
        It's look nicer if he get as output:




        Roll the dice! We got 1, 1, 5, 1, 5, for total of 1100 points




        What's next?



        Maybe a good challenge is to try implementing the full game based on complete rules? (this or this)






        share|improve this answer

























          up vote
          2
          down vote










          up vote
          2
          down vote









          Separation



          Good attempt on separation of concerns, but I think you can go further.




          • You can wrap the logic to generate a random integer in a function generate_random_int (int min, int max); where the content might looks like the example provided by Bjarne Stroustrup (look at rand_int) and so, no more globals.

          • You can wrap the logic about filling a container with random numbers in a function template<class ForwardIt> void fill_random(ForwardIt begin, ForwardIt end) (which internally can rely on std::generate and your generate_random_int).


          Be explicit



          Try to be explicit about "how" and "what"





          • How: Use self-explanary methods and algorithms.


          • What: Uses names that make sense


          Parameters type



          Try to pass types that are not cheap to copy, by const& (unless you need "destructive" work on it).



          Map initialization



          You don't need to initialize the map with 0's because when you try accessing to a key that doesn't exist, it is default constructed (so with 0's here).



          Choose right types



          As stated in other answers since your map's keys is a range of integers, you should use a std::vector or even since you know it size at compile time, std::array that you automatically fill with 0's when you default construct: std::array <int, 7> counts{};. (If you don't want to waste space of the elem at index 0, you have to do some computation later).



          Occurrences counting



          When you write:



          if (some_var == 3) { do_something(); }             //A
          if (some_var == 2) { do_something_else(); } //B


          If A is true, B can never be true. Instead of re-checking when it's useless, simply use else if :



          if (some_var == 3) { do_something(); } 
          else if (some_var == 3) { do_something_else(); }


          But...



          ... Instead of testing several times for each dice side possible count, you can reduce branchements checking all "combo" first:



          if (counts[1] >= 3) {
          result += 1000;
          counts[1] -= 3; // here we decrement to remove the wombo combo from the count
          }
          else if (counts[2] >= 3) {
          //...
          }
          // ...
          if (counts[1] > 1) {
          result += 100 * counts[1];
          }
          // ...


          Or even, automatically compute combo count



          // superbonus
          if (counts[1] >= 3) {
          result += 1000;
          counts [1] -= 3;
          }
          // combo bonus
          else {
          for (int index = 2; index < 7; ++index) {
          if (counts[index] >= 3) {
          result += index * 100;
          counts[index] -= 3;
          break;
          }
          }
          }
          // ...
          if (counts[1] > 1) {
          result += counts[1] * 100;
          }
          // ...


          Or, maybe more explicit:



          // combo bonus
          for (int index = 1; index < 7; ++index) {
          if (counts[index] >= 3) {
          result += index * 100;
          counts[index] -= 3;
          break;
          }
          }
          if (result == 100) {
          result *= 10; // superbonus
          }
          if (counts[1] > 1) {
          result += counts[1] * 100;
          }
          // ...


          Or using the @Snowhawk method, more imperative (and surely more efficient) but less flexible if you want to change your algorithm later.



          User-friendly output



          Instead of just printing the output, add some information to the user.
          It's look nicer if he get as output:




          Roll the dice! We got 1, 1, 5, 1, 5, for total of 1100 points




          What's next?



          Maybe a good challenge is to try implementing the full game based on complete rules? (this or this)






          share|improve this answer














          Separation



          Good attempt on separation of concerns, but I think you can go further.




          • You can wrap the logic to generate a random integer in a function generate_random_int (int min, int max); where the content might looks like the example provided by Bjarne Stroustrup (look at rand_int) and so, no more globals.

          • You can wrap the logic about filling a container with random numbers in a function template<class ForwardIt> void fill_random(ForwardIt begin, ForwardIt end) (which internally can rely on std::generate and your generate_random_int).


          Be explicit



          Try to be explicit about "how" and "what"





          • How: Use self-explanary methods and algorithms.


          • What: Uses names that make sense


          Parameters type



          Try to pass types that are not cheap to copy, by const& (unless you need "destructive" work on it).



          Map initialization



          You don't need to initialize the map with 0's because when you try accessing to a key that doesn't exist, it is default constructed (so with 0's here).



          Choose right types



          As stated in other answers since your map's keys is a range of integers, you should use a std::vector or even since you know it size at compile time, std::array that you automatically fill with 0's when you default construct: std::array <int, 7> counts{};. (If you don't want to waste space of the elem at index 0, you have to do some computation later).



          Occurrences counting



          When you write:



          if (some_var == 3) { do_something(); }             //A
          if (some_var == 2) { do_something_else(); } //B


          If A is true, B can never be true. Instead of re-checking when it's useless, simply use else if :



          if (some_var == 3) { do_something(); } 
          else if (some_var == 3) { do_something_else(); }


          But...



          ... Instead of testing several times for each dice side possible count, you can reduce branchements checking all "combo" first:



          if (counts[1] >= 3) {
          result += 1000;
          counts[1] -= 3; // here we decrement to remove the wombo combo from the count
          }
          else if (counts[2] >= 3) {
          //...
          }
          // ...
          if (counts[1] > 1) {
          result += 100 * counts[1];
          }
          // ...


          Or even, automatically compute combo count



          // superbonus
          if (counts[1] >= 3) {
          result += 1000;
          counts [1] -= 3;
          }
          // combo bonus
          else {
          for (int index = 2; index < 7; ++index) {
          if (counts[index] >= 3) {
          result += index * 100;
          counts[index] -= 3;
          break;
          }
          }
          }
          // ...
          if (counts[1] > 1) {
          result += counts[1] * 100;
          }
          // ...


          Or, maybe more explicit:



          // combo bonus
          for (int index = 1; index < 7; ++index) {
          if (counts[index] >= 3) {
          result += index * 100;
          counts[index] -= 3;
          break;
          }
          }
          if (result == 100) {
          result *= 10; // superbonus
          }
          if (counts[1] > 1) {
          result += counts[1] * 100;
          }
          // ...


          Or using the @Snowhawk method, more imperative (and surely more efficient) but less flexible if you want to change your algorithm later.



          User-friendly output



          Instead of just printing the output, add some information to the user.
          It's look nicer if he get as output:




          Roll the dice! We got 1, 1, 5, 1, 5, for total of 1100 points




          What's next?



          Maybe a good challenge is to try implementing the full game based on complete rules? (this or this)







          share|improve this answer














          share|improve this answer



          share|improve this answer








          edited 3 hours ago

























          answered 5 hours ago









          Calak

          1,45912




          1,45912






















              up vote
              1
              down vote













              std::random_device seeder;
              std::mt19937 engine(seeder());


              While this is fine for small hobby projects or testing, you'll likely want to properly seed std::mt19937. Let's break this down into explicit steps to understand what is going on.



              std::random_device rdev;     


              std::random_device asks the OS for system-provided randomness. It can be, and sometimes is, implemented as a simple PRNG with a fixed seed, meaning you could produce the same sequence on every run. Arguably worse than using time(NULL) as a source for entropy.



              auto random_seed{rdev()};


              Invoking the random device object returns an unsigned int. This is normally 4 bytes, but it could be 2.



              std::seed_seq seeder{random_seed};


              A seed sequence is created using that one 2/4-byte value.



              std::mt19937 engine(seeder);


              You are now attempting to initialize the internal 624 32-bit integer state of the Mersenne Twister with that one value. This leads to issues related to




              • predictability - searching for the seed is simple as there are only 2^32 possibilities.

              • bias - values like 7 and 13 will never be generated. Two different seeds produce 0, 12 different seeds produce 1226181350.


              If you are interested the perils/pitfalls of random bit generation and std::seed_seq, read through the comments here.





               typedef std::vector<int> list_type;


              If you are expecting a fixed length container, consider using std::array over std::vector.





                  std::map<int, int> cnt;


              std::map is overkill for counting a contiguous range of values. std::unordered_map is better. An array-based counting sort would be best.



                  for (int i = 1; i <= 6; ++i)
              {
              cnt[i] = 0;
              }

              for (auto &d : die_rolls)
              {
              ++cnt[d];
              }


              std::map and std::unordered_map will default construct a value into the container if the key doesn't exist. You can skip the value initialization for these containers.





                  if (cnt[1] == 3) { ret += 1000; }
              /* ... */


              You can do some strength reduction here by reorganizing the related comparisons and converting some of the branches into integer multiplications. Since you limit the number of dice to five, you can immediately stop checking the remaining triples once the first triple has been found.



                  if (cnt[1] >= 3) { score += 1000; cnt[1] -= 3; }
              else if (cnt[2] >= 3) { score += 200; }
              else if (cnt[3] >= 3) { score += 300; }
              else if (cnt[4] >= 3) { score += 400; }
              else if (cnt[5] >= 3) { score += 500; cnt[5] -= 3; }
              else if (cnt[6] >= 3) { score += 600; }

              score += cnt[1] * 100 + cnt[5] * 50;





              share|improve this answer























              • Very interesting explanation about randomization!
                – Calak
                5 hours ago






              • 1




                You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
                – Calak
                4 hours ago















              up vote
              1
              down vote













              std::random_device seeder;
              std::mt19937 engine(seeder());


              While this is fine for small hobby projects or testing, you'll likely want to properly seed std::mt19937. Let's break this down into explicit steps to understand what is going on.



              std::random_device rdev;     


              std::random_device asks the OS for system-provided randomness. It can be, and sometimes is, implemented as a simple PRNG with a fixed seed, meaning you could produce the same sequence on every run. Arguably worse than using time(NULL) as a source for entropy.



              auto random_seed{rdev()};


              Invoking the random device object returns an unsigned int. This is normally 4 bytes, but it could be 2.



              std::seed_seq seeder{random_seed};


              A seed sequence is created using that one 2/4-byte value.



              std::mt19937 engine(seeder);


              You are now attempting to initialize the internal 624 32-bit integer state of the Mersenne Twister with that one value. This leads to issues related to




              • predictability - searching for the seed is simple as there are only 2^32 possibilities.

              • bias - values like 7 and 13 will never be generated. Two different seeds produce 0, 12 different seeds produce 1226181350.


              If you are interested the perils/pitfalls of random bit generation and std::seed_seq, read through the comments here.





               typedef std::vector<int> list_type;


              If you are expecting a fixed length container, consider using std::array over std::vector.





                  std::map<int, int> cnt;


              std::map is overkill for counting a contiguous range of values. std::unordered_map is better. An array-based counting sort would be best.



                  for (int i = 1; i <= 6; ++i)
              {
              cnt[i] = 0;
              }

              for (auto &d : die_rolls)
              {
              ++cnt[d];
              }


              std::map and std::unordered_map will default construct a value into the container if the key doesn't exist. You can skip the value initialization for these containers.





                  if (cnt[1] == 3) { ret += 1000; }
              /* ... */


              You can do some strength reduction here by reorganizing the related comparisons and converting some of the branches into integer multiplications. Since you limit the number of dice to five, you can immediately stop checking the remaining triples once the first triple has been found.



                  if (cnt[1] >= 3) { score += 1000; cnt[1] -= 3; }
              else if (cnt[2] >= 3) { score += 200; }
              else if (cnt[3] >= 3) { score += 300; }
              else if (cnt[4] >= 3) { score += 400; }
              else if (cnt[5] >= 3) { score += 500; cnt[5] -= 3; }
              else if (cnt[6] >= 3) { score += 600; }

              score += cnt[1] * 100 + cnt[5] * 50;





              share|improve this answer























              • Very interesting explanation about randomization!
                – Calak
                5 hours ago






              • 1




                You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
                – Calak
                4 hours ago













              up vote
              1
              down vote










              up vote
              1
              down vote









              std::random_device seeder;
              std::mt19937 engine(seeder());


              While this is fine for small hobby projects or testing, you'll likely want to properly seed std::mt19937. Let's break this down into explicit steps to understand what is going on.



              std::random_device rdev;     


              std::random_device asks the OS for system-provided randomness. It can be, and sometimes is, implemented as a simple PRNG with a fixed seed, meaning you could produce the same sequence on every run. Arguably worse than using time(NULL) as a source for entropy.



              auto random_seed{rdev()};


              Invoking the random device object returns an unsigned int. This is normally 4 bytes, but it could be 2.



              std::seed_seq seeder{random_seed};


              A seed sequence is created using that one 2/4-byte value.



              std::mt19937 engine(seeder);


              You are now attempting to initialize the internal 624 32-bit integer state of the Mersenne Twister with that one value. This leads to issues related to




              • predictability - searching for the seed is simple as there are only 2^32 possibilities.

              • bias - values like 7 and 13 will never be generated. Two different seeds produce 0, 12 different seeds produce 1226181350.


              If you are interested the perils/pitfalls of random bit generation and std::seed_seq, read through the comments here.





               typedef std::vector<int> list_type;


              If you are expecting a fixed length container, consider using std::array over std::vector.





                  std::map<int, int> cnt;


              std::map is overkill for counting a contiguous range of values. std::unordered_map is better. An array-based counting sort would be best.



                  for (int i = 1; i <= 6; ++i)
              {
              cnt[i] = 0;
              }

              for (auto &d : die_rolls)
              {
              ++cnt[d];
              }


              std::map and std::unordered_map will default construct a value into the container if the key doesn't exist. You can skip the value initialization for these containers.





                  if (cnt[1] == 3) { ret += 1000; }
              /* ... */


              You can do some strength reduction here by reorganizing the related comparisons and converting some of the branches into integer multiplications. Since you limit the number of dice to five, you can immediately stop checking the remaining triples once the first triple has been found.



                  if (cnt[1] >= 3) { score += 1000; cnt[1] -= 3; }
              else if (cnt[2] >= 3) { score += 200; }
              else if (cnt[3] >= 3) { score += 300; }
              else if (cnt[4] >= 3) { score += 400; }
              else if (cnt[5] >= 3) { score += 500; cnt[5] -= 3; }
              else if (cnt[6] >= 3) { score += 600; }

              score += cnt[1] * 100 + cnt[5] * 50;





              share|improve this answer














              std::random_device seeder;
              std::mt19937 engine(seeder());


              While this is fine for small hobby projects or testing, you'll likely want to properly seed std::mt19937. Let's break this down into explicit steps to understand what is going on.



              std::random_device rdev;     


              std::random_device asks the OS for system-provided randomness. It can be, and sometimes is, implemented as a simple PRNG with a fixed seed, meaning you could produce the same sequence on every run. Arguably worse than using time(NULL) as a source for entropy.



              auto random_seed{rdev()};


              Invoking the random device object returns an unsigned int. This is normally 4 bytes, but it could be 2.



              std::seed_seq seeder{random_seed};


              A seed sequence is created using that one 2/4-byte value.



              std::mt19937 engine(seeder);


              You are now attempting to initialize the internal 624 32-bit integer state of the Mersenne Twister with that one value. This leads to issues related to




              • predictability - searching for the seed is simple as there are only 2^32 possibilities.

              • bias - values like 7 and 13 will never be generated. Two different seeds produce 0, 12 different seeds produce 1226181350.


              If you are interested the perils/pitfalls of random bit generation and std::seed_seq, read through the comments here.





               typedef std::vector<int> list_type;


              If you are expecting a fixed length container, consider using std::array over std::vector.





                  std::map<int, int> cnt;


              std::map is overkill for counting a contiguous range of values. std::unordered_map is better. An array-based counting sort would be best.



                  for (int i = 1; i <= 6; ++i)
              {
              cnt[i] = 0;
              }

              for (auto &d : die_rolls)
              {
              ++cnt[d];
              }


              std::map and std::unordered_map will default construct a value into the container if the key doesn't exist. You can skip the value initialization for these containers.





                  if (cnt[1] == 3) { ret += 1000; }
              /* ... */


              You can do some strength reduction here by reorganizing the related comparisons and converting some of the branches into integer multiplications. Since you limit the number of dice to five, you can immediately stop checking the remaining triples once the first triple has been found.



                  if (cnt[1] >= 3) { score += 1000; cnt[1] -= 3; }
              else if (cnt[2] >= 3) { score += 200; }
              else if (cnt[3] >= 3) { score += 300; }
              else if (cnt[4] >= 3) { score += 400; }
              else if (cnt[5] >= 3) { score += 500; cnt[5] -= 3; }
              else if (cnt[6] >= 3) { score += 600; }

              score += cnt[1] * 100 + cnt[5] * 50;






              share|improve this answer














              share|improve this answer



              share|improve this answer








              edited 3 hours ago









              Calak

              1,45912




              1,45912










              answered 7 hours ago









              Snowhawk

              4,99911028




              4,99911028












              • Very interesting explanation about randomization!
                – Calak
                5 hours ago






              • 1




                You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
                – Calak
                4 hours ago


















              • Very interesting explanation about randomization!
                – Calak
                5 hours ago






              • 1




                You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
                – Calak
                4 hours ago
















              Very interesting explanation about randomization!
              – Calak
              5 hours ago




              Very interesting explanation about randomization!
              – Calak
              5 hours ago




              1




              1




              You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
              – Calak
              4 hours ago




              You should modify == for >= because if we have more than 3 same values we still have at least 3 values so a combo ;)
              – Calak
              4 hours ago










              up vote
              0
              down vote













              You can easily decouple the scoring-algorithm from the container used to score dice-rolls by using templates. Not that you actually need a container, if you have the right iterators.



              Anyway, why aren't you prepared to deal with more dice being thrown?



              Also, consider making it data-driven:



              template <class InputIt, class Sentinel = InputIt>
              auto score(InputIt first, Sentinel last) {
              constexpr auto cap = 16;
              constexpr unsigned goals[3] = {
              {1, 3, 1000},
              {6, 3, 600},
              {5, 3, 500},
              {4, 3, 400},
              {3, 3, 300},
              {2, 3, 200},
              {1, 1, 100},
              {5, 1, 50},
              };
              unsigned char dice[cap] = {};
              for (; first != last; ++first)
              ++dice[(unsigned)*first % cap];
              auto result = 0ULL;
              for (auto [which, count, reward] : goals) {
              auto& x = dice[which % cap];
              result += x / count * reward;
              x %= count;
              }
              return result;
              }


              Global mutable state is best avoided. Why is the random-generator and related things global?



              For some reason, you have extra-newlines surrounding the return of your scoring-function. Maybe you should automate indentation?



              main() is your only function where the opening brace doesn't have its own line. While there is Nothing wrong with that, try for consistency.



              return 0; is implicit for main().





              share

























                up vote
                0
                down vote













                You can easily decouple the scoring-algorithm from the container used to score dice-rolls by using templates. Not that you actually need a container, if you have the right iterators.



                Anyway, why aren't you prepared to deal with more dice being thrown?



                Also, consider making it data-driven:



                template <class InputIt, class Sentinel = InputIt>
                auto score(InputIt first, Sentinel last) {
                constexpr auto cap = 16;
                constexpr unsigned goals[3] = {
                {1, 3, 1000},
                {6, 3, 600},
                {5, 3, 500},
                {4, 3, 400},
                {3, 3, 300},
                {2, 3, 200},
                {1, 1, 100},
                {5, 1, 50},
                };
                unsigned char dice[cap] = {};
                for (; first != last; ++first)
                ++dice[(unsigned)*first % cap];
                auto result = 0ULL;
                for (auto [which, count, reward] : goals) {
                auto& x = dice[which % cap];
                result += x / count * reward;
                x %= count;
                }
                return result;
                }


                Global mutable state is best avoided. Why is the random-generator and related things global?



                For some reason, you have extra-newlines surrounding the return of your scoring-function. Maybe you should automate indentation?



                main() is your only function where the opening brace doesn't have its own line. While there is Nothing wrong with that, try for consistency.



                return 0; is implicit for main().





                share























                  up vote
                  0
                  down vote










                  up vote
                  0
                  down vote









                  You can easily decouple the scoring-algorithm from the container used to score dice-rolls by using templates. Not that you actually need a container, if you have the right iterators.



                  Anyway, why aren't you prepared to deal with more dice being thrown?



                  Also, consider making it data-driven:



                  template <class InputIt, class Sentinel = InputIt>
                  auto score(InputIt first, Sentinel last) {
                  constexpr auto cap = 16;
                  constexpr unsigned goals[3] = {
                  {1, 3, 1000},
                  {6, 3, 600},
                  {5, 3, 500},
                  {4, 3, 400},
                  {3, 3, 300},
                  {2, 3, 200},
                  {1, 1, 100},
                  {5, 1, 50},
                  };
                  unsigned char dice[cap] = {};
                  for (; first != last; ++first)
                  ++dice[(unsigned)*first % cap];
                  auto result = 0ULL;
                  for (auto [which, count, reward] : goals) {
                  auto& x = dice[which % cap];
                  result += x / count * reward;
                  x %= count;
                  }
                  return result;
                  }


                  Global mutable state is best avoided. Why is the random-generator and related things global?



                  For some reason, you have extra-newlines surrounding the return of your scoring-function. Maybe you should automate indentation?



                  main() is your only function where the opening brace doesn't have its own line. While there is Nothing wrong with that, try for consistency.



                  return 0; is implicit for main().





                  share












                  You can easily decouple the scoring-algorithm from the container used to score dice-rolls by using templates. Not that you actually need a container, if you have the right iterators.



                  Anyway, why aren't you prepared to deal with more dice being thrown?



                  Also, consider making it data-driven:



                  template <class InputIt, class Sentinel = InputIt>
                  auto score(InputIt first, Sentinel last) {
                  constexpr auto cap = 16;
                  constexpr unsigned goals[3] = {
                  {1, 3, 1000},
                  {6, 3, 600},
                  {5, 3, 500},
                  {4, 3, 400},
                  {3, 3, 300},
                  {2, 3, 200},
                  {1, 1, 100},
                  {5, 1, 50},
                  };
                  unsigned char dice[cap] = {};
                  for (; first != last; ++first)
                  ++dice[(unsigned)*first % cap];
                  auto result = 0ULL;
                  for (auto [which, count, reward] : goals) {
                  auto& x = dice[which % cap];
                  result += x / count * reward;
                  x %= count;
                  }
                  return result;
                  }


                  Global mutable state is best avoided. Why is the random-generator and related things global?



                  For some reason, you have extra-newlines surrounding the return of your scoring-function. Maybe you should automate indentation?



                  main() is your only function where the opening brace doesn't have its own line. While there is Nothing wrong with that, try for consistency.



                  return 0; is implicit for main().






                  share











                  share


                  share










                  answered 59 secs ago









                  Deduplicator

                  10.8k1849




                  10.8k1849






















                      ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.










                       

                      draft saved


                      draft discarded


















                      ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.













                      ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.












                      ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.















                       


                      draft saved


                      draft discarded














                      StackExchange.ready(
                      function () {
                      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207689%2fa-dice-game-called-greed%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

                      Mont Emei

                      Province de Neuquén

                      Journaliste