HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Pokemon stats calculator

Submitted by: @import:stackexchange-codereview··
0
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 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.

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.