A dice game called “Greed”
up vote
9
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)
.
c++ game random c++14 dice
New contributor
add a comment |
up vote
9
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)
.
c++ game random c++14 dice
New contributor
4
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 calledgreed
, wouldn'tcalculate_score
androll_dice
be way more explanatory as function names?
– sbecker
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 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
7 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago
add a comment |
up vote
9
down vote
favorite
up vote
9
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)
.
c++ game random c++14 dice
New contributor
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
c++ game random c++14 dice
New contributor
New contributor
edited 6 hours ago
Toby Speight
21.8k536107
21.8k536107
New contributor
asked 20 hours ago
ChubakBidpaa
925
925
New contributor
New contributor
4
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 calledgreed
, wouldn'tcalculate_score
androll_dice
be way more explanatory as function names?
– sbecker
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 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
7 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago
add a comment |
4
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 calledgreed
, wouldn'tcalculate_score
androll_dice
be way more explanatory as function names?
– sbecker
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 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
7 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago
4
4
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
12 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
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 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
7 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
7 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago
add a comment |
5 Answers
5
active
oldest
votes
up vote
13
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];
}
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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
add a comment |
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.
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why usingstd::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)
– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
add a comment |
up vote
4
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 atrand_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 onstd::generate
and yourgenerate_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)
add a comment |
up vote
3
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;
Very interesting explanation about randomization!
– Calak
9 hours ago
2
You should modify==
for>=
because if we have more than 3 same values we still have at least 3 values so a combo ;)
– Calak
7 hours ago
add a comment |
up vote
3
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 = 8;
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()
.
What's the need for thecap
here - it looks like you're taking an input that could be16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.
– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose ofx %= count;
or even... what are you doing withx
after this operation? And, are you sure about your computation inresult += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)
– Calak
30 mins ago
@Calak Well,x / count
is unsigned integer-arithmetic, so rounded down.* reward
is obvious. Andx %= count;
removes the dice just used.
– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence5, 21, 37
it will (in my opinion) incorrectly give the score 500.
– cmh
24 mins ago
|
show 6 more comments
5 Answers
5
active
oldest
votes
5 Answers
5
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
13
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];
}
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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
add a comment |
up vote
13
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];
}
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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
add a comment |
up vote
13
down vote
up vote
13
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];
}
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];
}
edited 20 hours ago
answered 20 hours ago
Errorsatz
4687
4687
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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
add a comment |
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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 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
20 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
20 hours ago
Note that your double loops for "counting logic" could probably be replaced by:
score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
Note that your double loops for "counting logic" could probably be replaced by:
score += 1000 * (count[1] / 3); score += 100 * (count[1] % 3);
– scohe001
3 hours ago
add a comment |
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.
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why usingstd::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)
– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
add a comment |
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.
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why usingstd::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)
– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
add a comment |
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.
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.
edited 15 hours ago
answered 19 hours ago
Cris Luengo
2,349319
2,349319
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why usingstd::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)
– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
add a comment |
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why usingstd::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)
– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
I didn't know about fill(). Thank you.
– ChubakBidpaa
17 hours ago
@CrisLuengo disagree. Why using
std::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)– Calak
15 hours ago
@CrisLuengo disagree. Why using
std::fill (...)
when you can just default construct? (std::array <int, 7> counts{};
)– Calak
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
@Calak: Good point! Answer updated.
– Cris Luengo
15 hours ago
add a comment |
up vote
4
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 atrand_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 onstd::generate
and yourgenerate_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)
add a comment |
up vote
4
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 atrand_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 onstd::generate
and yourgenerate_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)
add a comment |
up vote
4
down vote
up vote
4
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 atrand_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 onstd::generate
and yourgenerate_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)
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 atrand_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 onstd::generate
and yourgenerate_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)
edited 7 hours ago
answered 9 hours ago
Calak
1,47912
1,47912
add a comment |
add a comment |
up vote
3
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;
Very interesting explanation about randomization!
– Calak
9 hours ago
2
You should modify==
for>=
because if we have more than 3 same values we still have at least 3 values so a combo ;)
– Calak
7 hours ago
add a comment |
up vote
3
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;
Very interesting explanation about randomization!
– Calak
9 hours ago
2
You should modify==
for>=
because if we have more than 3 same values we still have at least 3 values so a combo ;)
– Calak
7 hours ago
add a comment |
up vote
3
down vote
up vote
3
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;
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;
edited 7 hours ago
Calak
1,47912
1,47912
answered 11 hours ago
Snowhawk
5,01911028
5,01911028
Very interesting explanation about randomization!
– Calak
9 hours ago
2
You should modify==
for>=
because if we have more than 3 same values we still have at least 3 values so a combo ;)
– Calak
7 hours ago
add a comment |
Very interesting explanation about randomization!
– Calak
9 hours ago
2
You should modify==
for>=
because if we have more than 3 same values we still have at least 3 values so a combo ;)
– Calak
7 hours ago
Very interesting explanation about randomization!
– Calak
9 hours ago
Very interesting explanation about randomization!
– Calak
9 hours ago
2
2
You should modify
==
for >=
because if we have more than 3 same values we still have at least 3 values so a combo ;)– Calak
7 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
7 hours ago
add a comment |
up vote
3
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 = 8;
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()
.
What's the need for thecap
here - it looks like you're taking an input that could be16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.
– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose ofx %= count;
or even... what are you doing withx
after this operation? And, are you sure about your computation inresult += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)
– Calak
30 mins ago
@Calak Well,x / count
is unsigned integer-arithmetic, so rounded down.* reward
is obvious. Andx %= count;
removes the dice just used.
– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence5, 21, 37
it will (in my opinion) incorrectly give the score 500.
– cmh
24 mins ago
|
show 6 more comments
up vote
3
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 = 8;
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()
.
What's the need for thecap
here - it looks like you're taking an input that could be16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.
– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose ofx %= count;
or even... what are you doing withx
after this operation? And, are you sure about your computation inresult += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)
– Calak
30 mins ago
@Calak Well,x / count
is unsigned integer-arithmetic, so rounded down.* reward
is obvious. Andx %= count;
removes the dice just used.
– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence5, 21, 37
it will (in my opinion) incorrectly give the score 500.
– cmh
24 mins ago
|
show 6 more comments
up vote
3
down vote
up vote
3
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 = 8;
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()
.
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 = 8;
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()
.
edited 22 mins ago
answered 3 hours ago
Deduplicator
10.8k1849
10.8k1849
What's the need for thecap
here - it looks like you're taking an input that could be16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.
– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose ofx %= count;
or even... what are you doing withx
after this operation? And, are you sure about your computation inresult += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)
– Calak
30 mins ago
@Calak Well,x / count
is unsigned integer-arithmetic, so rounded down.* reward
is obvious. Andx %= count;
removes the dice just used.
– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence5, 21, 37
it will (in my opinion) incorrectly give the score 500.
– cmh
24 mins ago
|
show 6 more comments
What's the need for thecap
here - it looks like you're taking an input that could be16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.
– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose ofx %= count;
or even... what are you doing withx
after this operation? And, are you sure about your computation inresult += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)
– Calak
30 mins ago
@Calak Well,x / count
is unsigned integer-arithmetic, so rounded down.* reward
is obvious. Andx %= count;
removes the dice just used.
– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence5, 21, 37
it will (in my opinion) incorrectly give the score 500.
– cmh
24 mins ago
What's the need for the
cap
here - it looks like you're taking an input that could be 16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.– cmh
1 hour ago
What's the need for the
cap
here - it looks like you're taking an input that could be 16k + face
, and provisioning for dice with faces up to 16? Perhaps I've just missed a detail.– cmh
1 hour ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
@cmh I'm taking 16 because that's the nearest higher power of two.
– Deduplicator
32 mins ago
I surely missed something. What's the purpose of
x %= count;
or even... what are you doing with x
after this operation? And, are you sure about your computation in result += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)– Calak
30 mins ago
I surely missed something. What's the purpose of
x %= count;
or even... what are you doing with x
after this operation? And, are you sure about your computation in result += x / count * reward;
? Compare his result and yours if dice are "1, 1, 1, 1, 2". I don't understand, maybe too tired :)– Calak
30 mins ago
@Calak Well,
x / count
is unsigned integer-arithmetic, so rounded down. * reward
is obvious. And x %= count;
removes the dice just used.– Deduplicator
24 mins ago
@Calak Well,
x / count
is unsigned integer-arithmetic, so rounded down. * reward
is obvious. And x %= count;
removes the dice just used.– Deduplicator
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence
5, 21, 37
it will (in my opinion) incorrectly give the score 500.– cmh
24 mins ago
@Deduplicator I suppose my confusion is that if I pass the sequence
5, 21, 37
it will (in my opinion) incorrectly give the score 500.– cmh
24 mins ago
|
show 6 more comments
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.
ChubakBidpaa is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
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
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function () {
StackExchange.helpers.onClickDraftSave('#login-link');
});
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
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
4
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'tcalculate_score
androll_dice
be way more explanatory as function names?– sbecker
12 hours ago
@sbecker You're right, from now on I'll choose better function and variable names.
– ChubakBidpaa
12 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
7 hours ago
I know this game by another name but I cant recall what it is.
– Matt
2 hours ago