Pokemon stats calculator

Question

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 <array>
#include <algorithm>
#include <iostream>
#include <cmath>
#include <cstdint>
#include <random>
#include <limits>

/* Satisfies UniformRandomNumberGenerator concept */
struct PokemonRandEngine
{
    using result_type = uint32_t;
    result_type min() { return std::numeric_limits<result_type>::min(); }
    result_type max() { return std::numeric_limits<result_type>::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;
    uint8_t spc; /* special */
};

struct pokemon
{
    std::string name;
    stats_t stats;
    stats_t ivs;
    int level;

    void populate_ivs()
    {
        static std::uniform_int_distribution<int8_t> uid(0, 15);
        static PokemonRandEngine cre;
        ivs.atk = uid(cre);
        ivs.def = uid(cre);
        ivs.spd = uid(cre);
        ivs.spc = uid(cre);
        /* The HP IV is calculated by taking the least significant bit
         * of each IV and then constructing a binary string
         */
        ivs.hp = (ivs.atk & 1) | (ivs.def & 1) | (ivs.spd & 1) | (ivs.spc & 1);
    }

    uint8_t calculate_hp(uint8_t base, uint8_t iv)
    {
        return floor(
                (
                    ((base + iv) * 2
                    + floor(ceil(sqrt(0))/4)) /* Assume 0 for Effort Values for now */
                    * level
                ) / 100
            )
            + level
            + 10;
    }

    uint8_t calculate_otherstat(uint8_t base, uint8_t iv)
    {
        return floor(
                (
                    ((base + iv) * 2
                    + floor(ceil(sqrt(0))/4)) /* Assume 0 for Effort Values for now */
                    * level
                ) / 100
            )
            + 5;
    }    

    void calculate_stats()
    {
        extern std::array<pokemon, 1> base_stats_array;
        stats_t base_stats = 
            std::find_if(base_stats_array.begin(), base_stats_array.end(),
            [this] (pokemon p) { return p.name == name; })->stats;
        stats.hp = calculate_hp(base_stats.hp, ivs.hp);
        stats.atk = calculate_otherstat(base_stats.atk, ivs.atk);
        stats.def = calculate_otherstat(base_stats.def, ivs.def);
        stats.spd = calculate_otherstat(base_stats.spd, ivs.spd);
        stats.spc = calculate_otherstat(base_stats.spc, ivs.spc);
    }
};

std::array<pokemon, 1> base_stats_array = {
    { { "mewtwo", { 106, 110, 90, 130, 154 } } }
};

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;
}

Output:

234 178 138 198 229
228 161 139 201 237

Answer 1

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.

Answer 2

Yes, your random number generator does seem to meet the UniformRandomNumberGenerator description.

Sorry that this is so negative, your code looks more like C than C++.

Instead of using C casts like (int) prefer static_cast.

std::cout << (int)wild.stats.hp << " " << (int)wild.stats.atk << " " << (int)wild.stats.def << " " << (int)wild.stats.spd << " " <<  (int)wild.stats.spc;

Should be:

std::cout << static_cast<int>(wild.stats.hp) << " " << static_cast<int>(wild.stats.atk) << " " << static_cast<int>(wild.stats.def) << " " << static_cast<int>(wild.stats.spd) << " " << static_cast<int>(wild.stats.spc);

I would use class over struct. See: When should you use a class vs a struct in C++?

Your stat_t class should have an << operator to output the various statistics. That way the two lines in main would change to:

std::out << wild.stats << std::endl;

The names of your attributes are too short and unclear. What is ivs? What are the different attributes within the stat_t struct?

Where is the constructor for your pokemon class? The pokemon constructor should take the different parameters in main:

int main()
{
    pokemon wild;
    wild.name = "mewtwo";
    wild.level = 70;
    wild.ivs = { 4, 14, 5, 8, 6 };
    wild.calculate_stats();
}

Could look like:

stat_t tmpIvs(4, 14, 5, 8, 6);      // stat_t needs a constructor....
pokemon wild("mewtwo", 70, tmpIvs); // pokemon needs a constructor....

Answer 3

The other answers already provided good feedback and I agree that you should consider better naming and be more consistent with the naming style for the classes/structs. In particular, avoid excessively abbreviated names. Just spell out things like special, speed, attack, etc.

Also, do use constructors to initialize your data. As it stands, you leave room for uninitialized data bugs, since some of your struct members are not default initialized.

A few other points:

1. I suggest using a class for PokemonRandEngine. It already has some private data and methods. The general convention is to use structs for Plain-Old-Data (POD) aggregates that have only public data and no behavior (methods).

2. The min/max methods of PokemonRandEngine could be static. They don't require access to any member state. Or at the very least, they should be marked const.

3. Speaking of const methods, take a look on when to mark methods const: What is a const member function?

4. The base_stats_array also appears that it should have been a const, unless you expect to change its data during runtime. I would also move its declaration to the top of the file to avoid having to redeclare extern references to it, which can lead to annoying linker errors if a typo is introduced in one of the declarations. Just avoid repetition when you can.

5. Check the returned iterator of find() before dereferencing it:

   stats_t base_stats = 
        std::find_if(base_stats_array.begin(), base_stats_array.end(),
        [this] (pokemon p) { return p.name == name; })->stats;
   

   You are assuming that the function always succeeds, but if the element is not found, it will return base_stats_array.end(), which is not a valid iterator and cannot be dereferenced. If you're on a debug build and lucky, your compiler might trap with an assertion, or it might just crash mysteriously. Check the iterator before calling -> on it.

6. You have this code and comment in a couple places:

   floor(ceil(sqrt(0))/4)) /* Assume 0 for Effort Values for now */
   

   So let's instead move that "Effort" computation to a separate helper function, so in the future when it is actually replaced with some logic, you won't have to update multiple places:

   // FIXME: Assume 0 for Effort Values for now
   static double calc_effort() { return std::ceil(std::sqrt(0)); }
   

7. Strictly speaking, the math functions from <cmath> and the sized integral types from <cstding> belong to namespace std, so they should be std:: qualified. They work without the namespace prefix on most compilers because the C and C++ header files are usually shared, so the C headers have the names defined globally. This is not a requirement though, your code might fail to compile with a more strict compiler or even in a future update of your current one. A more portable program should just use the namespace prefix.