patterncppMinor
Pokemon stats calculator
Viewed 0 times
pokemonstatscalculator
Problem
I have a simple working (so it's not a hypothetical stub) framework for calculating Pokemon stats that will later be incorporated in a game. It uses the LCRNG from the first game in order to be as compatible as possible. I use formulas from the Bulbapedia Wiki. The example output matches the example given here.
Things I want to focus on:
-
Does the random engine satisfy the
-
I'm not working on embedded system but I use
-
Better way to organize my data structures. Right now I made the class an aggregate for easy initialization but that limits the usefulness of the class. I'm worried about maintainability and don't want to give the
-
The "extern" thing is terrible, but it's better than duplication with a "base_stats_class". Better way of handling it?
Of course, readability and other stuff are welcome criticism too.
```
#include
#include
#include
#include
#include
#include
#include
/ Satisfies UniformRandomNumberGenerator concept /
struct PokemonRandEngine
{
using result_type = uint32_t;
result_type min() { return std::numeric_limits::min(); }
result_type max() { return std::numeric_limits::max(); }
result_type g() { return prand(); }
result_type operator()()
{
return g();
}
PokemonRandEngine()
{
static std::random_device rd;
seed(rd());
}
private:
uint32_t next = 1;
/ Pokemon's LCRNG /
uint32_t prand()
{
next = (next * 0x41C64E6D) + 0x6073;
return next;
}
/ Seed prand /
void seed(uint32_t seed)
{
next = seed;
}
};
struct stats_t
{
uint8_t hp;
uint8_t atk;
uint8_t def;
uint8_t spd;
Things I want to focus on:
-
Does the random engine satisfy the
UniformRandomNumberGenerator concept correctly? Can its usability be improved? I don't want to go crazy and over-engineer it.-
I'm not working on embedded system but I use
uint8_t to make it explicit that the values have range [0, 255]. And there is some (tiny) bit manipulation as well. Or is it just better to use an int?-
Better way to organize my data structures. Right now I made the class an aggregate for easy initialization but that limits the usefulness of the class. I'm worried about maintainability and don't want to give the
Pokemon class too much responsibility.-
The "extern" thing is terrible, but it's better than duplication with a "base_stats_class". Better way of handling it?
Of course, readability and other stuff are welcome criticism too.
```
#include
#include
#include
#include
#include
#include
#include
/ Satisfies UniformRandomNumberGenerator concept /
struct PokemonRandEngine
{
using result_type = uint32_t;
result_type min() { return std::numeric_limits::min(); }
result_type max() { return std::numeric_limits::max(); }
result_type g() { return prand(); }
result_type operator()()
{
return g();
}
PokemonRandEngine()
{
static std::random_device rd;
seed(rd());
}
private:
uint32_t next = 1;
/ Pokemon's LCRNG /
uint32_t prand()
{
next = (next * 0x41C64E6D) + 0x6073;
return next;
}
/ Seed prand /
void seed(uint32_t seed)
{
next = seed;
}
};
struct stats_t
{
uint8_t hp;
uint8_t atk;
uint8_t def;
uint8_t spd;
Solution
The first thing that strikes me is a lack of readability and consistency.
Whats does
Why have you capitalized some structs, but not others?
Are the variable shortened to match the game? If not, full name may be advisable.
Also, perhaps wrapping
In my opinion, there is way too much code in main. Separate this out into clearly named functions. This will increase readability and make maintenance easier.
I would also change all these numbers to named variables.
struct stats_t
{
uint8_t hp;
uint8_t atk;
uint8_t def;
uint8_t spd;
uint8_t spc; /* special */
};Whats does
stats_t mean? Why not just name it Stats? _tseems to have pitfalls. See the question: How should I mark types in C and C++ programs?Why have you capitalized some structs, but not others?
Are the variable shortened to match the game? If not, full name may be advisable.
Also, perhaps wrapping
uint8_t into a class would make it easier to maintain.int main()
{
pokemon wild;
wild.name = "mewtwo";
wild.level = 70;
wild.ivs = { 4, 14, 5, 8, 6 };
wild.calculate_stats();
std::cout << (int)wild.stats.hp << " " << (int)wild.stats.atk << " " << (int)wild.stats.def << " " << (int)wild.stats.spd << " " << (int)wild.stats.spc;
std::cout << std::endl;
wild.ivs = { 0, 2, 6, 10, 12 };
wild.calculate_stats();
std::cout << (int)wild.stats.hp << " " << (int)wild.stats.atk << " " << (int)wild.stats.def << " " << (int)wild.stats.spd << " " << (int)wild.stats.spc;
}In my opinion, there is way too much code in main. Separate this out into clearly named functions. This will increase readability and make maintenance easier.
wild.ivs = { 4, 14, 5, 8, 6 };I would also change all these numbers to named variables.
wild.ivs = { health, attack, strength, ... };.ivs is also not a clear name in my opinion.Code Snippets
struct stats_t
{
uint8_t hp;
uint8_t atk;
uint8_t def;
uint8_t spd;
uint8_t spc; /* special */
};int main()
{
pokemon wild;
wild.name = "mewtwo";
wild.level = 70;
wild.ivs = { 4, 14, 5, 8, 6 };
wild.calculate_stats();
std::cout << (int)wild.stats.hp << " " << (int)wild.stats.atk << " " << (int)wild.stats.def << " " << (int)wild.stats.spd << " " << (int)wild.stats.spc;
std::cout << std::endl;
wild.ivs = { 0, 2, 6, 10, 12 };
wild.calculate_stats();
std::cout << (int)wild.stats.hp << " " << (int)wild.stats.atk << " " << (int)wild.stats.def << " " << (int)wild.stats.spd << " " << (int)wild.stats.spc;
}wild.ivs = { 4, 14, 5, 8, 6 };wild.ivs = { health, attack, strength, ... };Context
StackExchange Code Review Q#122786, answer score: 7
Revisions (0)
No revisions yet.