C++ Beginner Bunny Exercise

Question

I recently completed (somewhat) an exercise I found on a C++ forum. The exercise is found at the end of the list of exercises and is called "Graduation" and is about bunnies in a linked list.

Write a program that creates a linked list of bunny objects. Each bunny object must have

• Sex: Male, Female (random at creation 50/50)
• color: white, brown, black, spotted
• age : 0-10 (years old) [with the exception of mutant bunnies (0-50)]
• Name : randomly chosen at creation from a list of bunny names. radioactive_mutant_vampire_bunny: true/false (decided at time of bunny creation 2% chance of true)

---

• At program initialization 5 bunnies must be created and given random colors.
• Each turn afterwards the bunnies age 1 year.
• So long as there is at least one male age 2 or older, for each female bunny in the list age 2 or older; a new bunny is created each turn. (i.e. if there was 1 adult male and 3 adult female bunnies, three new bunnies would be born each turn)
• New bunnies born should be the same color as their mother.
• If a bunny becomes older than 10 years old, it dies [with the exception of mutant bunnies, which can become 50 years old].
• If a radioactive mutant vampire bunny is born then each turn it will change exactly one non radioactive bunny into a radioactive vampire bunny. (if there are two radioactive mutant vampire bunnies two bunnies will be changed each turn and so on...)
• Radioactive vampire bunnies are excluded from regular breeding and do not count as adult bunnies.
• Radioactive vampire bunnies do not die until they reach age 50. The program should print a list of all the bunnies in the colony each turn along w/ all the bunnies details, sorted by age.

• The program should also output each turns events such as

  Bunny Thumper was born!
  Bunny Fufu was born!
  Radioactive Mutant Vampire Bunny Darth Maul was born!
  Bunny Julius Caesar died!

• The program should write all screen output to a file.
• When all the bunnies have died the program terminates.
• If the bunny population exceeds 1000 a food shortage must occur killing exactly half of the bunnies (randomly chosen)

I've gotten almost all of it done minus the file and GUI part. I'm looking for feedback specifically on performance, decisions with function return values and parameters(ex: returning a list<Bunny> vs taking a list<Bunny> as a parameter and modifying it in the function), design of constructors and other member functions, and use of global variables. Really, anything that would help make me a better programmer would be greatly welcome.

Note: std_lib_facilities.h is simply a standard library access header from a book I use. Oh, and please excuse my lack of creativity with the names.

Main.cpp

#include "Game.h"
#include <Windows.h>

int main()
{
    srand(time(0));
    list<Bunny>bunnies;
    spawn_bunnies(bunnies);
    while (Bunny::count_total_bunnies > 0)
    {
        Sleep(2000);
        end_turn(bunnies);
        Sleep(1000);
    }
    cout << "All bunnies have died!" << endl;
}

Bunny.h

#include "../../../std_lib_facilities.h"

enum Sex {
    male, female
};

enum Color {
    white, brown, black, spotted
};

class Bunny {
private:
    Sex sex;
    Color color;
    int age;
    string name;
    bool radioactive_mutant_vampire_bunny;
    int max_age;
public:
    static int count_radioactive_bunnies;
    static int count_total_bunnies;
    Bunny();
    Bunny(Color c);
    void increment_age()
    {
        ++age;
    }
    int get_age() const
    {
        return age;
    }
    Sex get_sex() const
    {
        return sex;
    }
    Color get_color()
    {
        return color;
    }
    string get_name()
    {
        return name;
    }
    bool is_radioactive() const
    {
        return radioactive_mutant_vampire_bunny;
    }
    void set_radioactive(bool b)
    {
        radioactive_mutant_vampire_bunny = b;
    }
    int get_max_age()
    {
        return max_age;
    }
};

Bunny.cpp

#include "Bunny.h"
vector<string>bunny_names = { "Larry", "Bob", "Tom", "George", "Pal", "Joe", "Nick", "da Bunny"};
int Bunny::count_radioactive_bunnies = 0;
int Bunny::count_total_bunnies = 0;
Bunny::Bunny()
    :sex{ (rand() % 100 + 1) < 50 ? Sex::male : Sex::female },
    color{Color(rand() % 4)},
    age{ 0 },
    name{ bunny_names[rand() % bunny_names.size()] },
    radioactive_mutant_vampire_bunny{ (rand() % 100 + 1) <= 2 ? true : false },
    max_age{ radioactive_mutant_vampire_bunny ? 50 : 10 }
{
    ++count_total_bunnies;
    if (radioactive_mutant_vampire_bunny)
    {
        cout << "Radioactive Mutant Vampire Bunny " << get_name() << " was born!" << endl;
        ++count_radioactive_bunnies;
    }
    else {
        cout << "Bunny " << get_name() << " was born!\n";
    }
}
Bunny::Bunny(Color c)
    :sex{ (rand() % 100 + 1) < 50 ? Sex::male : Sex::female },
    color{ c },
    age{ 0 },
    name{ bunny_names[rand() % bunny_names.size()] },
    radioactive_mutant_vampire_bunny{ (rand() % 100 + 1) <= 2 ? true : false },
    max_age{ radioactive_mutant_vampire_bunny ? 50 : 10 }
{
    ++count_total_bunnies;
    if (radioactive_mutant_vampire_bunny)
    {
        cout << "Radioactive Mutant Vampire Bunny " << get_name() << " was born!" << endl;
        ++count_radioactive_bunnies;
    }
    else {
        cout << "Bunny " << get_name() << " was born!\n";
    }

}

Game.h

#include "Bunny.h"
void spawn_bunnies(list<Bunny>&b); //runs only once in the beginning to instantiate the first 5 bunny objects

void end_turn(list<Bunny>&); // runs every ~2 seconds

bool male_available(const list<Bunny>&); //checks if there is a "valid" male available to reproduce

list<Bunny> females_available(const list<Bunny>&); //returns list of "valid" females to reproduce

void create_babies(list<Bunny>&); //uses list of females_available to determine color of bunny babies, as well as the # of babies

void transform_random_bunny(list<Bunny>&b); //transforms n random normal bunnies into mutant bunnies. n = num of current mutant bunnies

void bunnies_die(list<Bunny>&); //kills half of the bunnies when their count is above 1000

list<Bunny> get_normal_bunnies(const list<Bunny>& b); //returns a list of normal bunnies

Game.cpp

#include "Game.h"
void spawn_bunnies(list<Bunny>&b)
{
    Bunny b1;
    Bunny b2;
    Bunny b3;
    Bunny b4;
    Bunny b5;
    b = { b1, b2, b3, b4, b5 };
}
void end_turn(list<Bunny>&b)
{
    list<Bunny>::iterator bunny = b.begin();
    while (bunny != b.end())
    {
        bunny->increment_age();
        if (bunny->get_age() > bunny->get_max_age())
        {
            cout << "Bunny " << bunny->get_name() << " died!" << endl;
            bunny = b.erase(bunny);
            --Bunny::count_total_bunnies;
        }
        else
        {
            ++bunny;
        }
    }
    create_babies(b);
    if (Bunny::count_radioactive_bunnies) transform_random_bunny(b);
    if (Bunny::count_total_bunnies > 1000) bunnies_die(b);
}
bool male_available(const list<Bunny>&b)
{
    for (const Bunny b : b)
    {
        if (b.get_age() >= 2 && b.get_sex() == Sex::male && !b.is_radioactive())
        {
            return true;
        }
    }
    return false;
}
list<Bunny> females_available(const list<Bunny>&b)
{
    list<Bunny>females;
    for (const Bunny& bunny : b)
    {
        if (bunny.get_age() >= 2 && bunny.get_sex() == Sex::female && !bunny.is_radioactive())
        {
            females.push_back(bunny);
        }
    }
    return females;
}
void create_babies(list<Bunny>& b)
{
    list<Bunny>females = females_available(b);
    if (male_available(b) && females.size())
    {
        for (Bunny p : females)
        {
            Bunny new_bunny{ p.get_color() };
            b.push_back(new_bunny);
        }
    }
}
void transform_random_bunny(list<Bunny>&b)
{
    list<Bunny> normal_bunnies = get_normal_bunnies(b);
    for (int i = 0; i <= Bunny::count_radioactive_bunnies; i++)
    {
        list<Bunny>::iterator bunny = normal_bunnies.begin();
        advance(bunny, rand() % normal_bunnies.size());
        bunny->set_radioactive(true);
        normal_bunnies.erase(bunny);
    }
}

list<Bunny> get_normal_bunnies(const list<Bunny>& b)
{
    list<Bunny>normal_bunnies;
    for (const Bunny bunny : b)
    {
        if (!bunny.is_radioactive())
        {
            normal_bunnies.push_back(bunny);
        }
    }
    return normal_bunnies;
}
void bunnies_die(list<Bunny>&b)
{
    int bunnies_dead = b.size() / 2;
    for (int i = 0; i < bunnies_dead; i++)
    {
        list<Bunny>::iterator dead_bunny = b.begin();
        advance(dead_bunny, rand() % b.size());
        b.erase(dead_bunny);
        --Bunny::count_total_bunnies;
    }
}

Answer 1

Includes

I figured your std_lib_facilities.h has some includes for vector, list, string and stuff. While you might find that helpful to only have one header , it's rather bad practice. A header should only include the stuff it really needs, no more. Otherwise you might create false dependencies. E.g. your Game.h is the only one that uses list, which it only gets through including bunny that doesn't need it. So if you change the includes in Bunny.h you might break Game.h.

Also every header should have include guards, to prevent double inclusion.

Avoid using std::endl

Don't use std::endl to print the end of a line to your stream. It flushes the stream unnecessarily and makes your output slower. Simply use \n. For more information see here.

Don't use using namespace std

Your std_lib_facilities header has a using namespace std somewhere. Don't use that, you are poluting the global namespace with a lot of simple and common names. It's also less readable that way. The name is the namespace is so short on purpose, so that's it not a hassle to write. Again, for more info on that matter read here.

Don't use magic numbers

You are using hard coded numbers in your code for specific checks (required age for reproduction and max population). While it's easy to see what they are used for it gets harder to change them later on, the more your program grows. Use named variables for them, so it's easier to understand and you can see where they are actually used.

Don't repeat yourself

Whenever you write the same code twice, you should ask yourself, how you can prevent that in the first place. There are multiple instance where you create random numbers in some interval or check whether an event happened with a certain chance. Make those two things functions that can be reused and more easily read. And while you're at it, replace the random number generator with a better one(you can watch this to see why rand() is considered very bad)). You could write a small namespace for such utility functions, e.g.:

#include <random>

namespace Random
{
    static std::random_device rd;
    static std::mt19937 mt(rd());

    /// Return a random double value between min and max.
    inline double real(double min, double max)
    {
        return std::uniform_real_distribution<double>(min, max)(rd);
    }

    /// Return a random integer value between min and max.    
    inline int integer(int min, int max)
    {
        return std::uniform_int_distribution<int>(min, max)(rd);
    }

    inline bool flip(double chance)
    {
        return Random::real(0, 100) <= chance;
    }
}

With this you can then replace your calls to rand() with,

advance(dead_bunny, Random::integer(0, b.size() - 1));

:sex{Random::flip(50) ? Sex::male : Sex::female},

Another thins is your constructors for Bunny. They do exactly the same thing, expect for the initialization of color. If you want to change it you always have to do it twice. You could simply redirect the calls as in:

Bunny::Bunny()
    : Bunny(Color(Random::integer(0, 3)))
{
}

The same goes for your checking for random events. Make that a function that can take a probability and returns true or false:

bool flip(double chance)
{
    return (rand() % 100 + 1) <= chance;
}

It's easier to write and read. The same applies for creating a random number in some interval. And while you're at it, you can also exchange the random number generator for a better one.

Simplifications

Your spwan_bunnies method is rather strange. If you later decide to start your population with 100 bunnies instead of 5, you would require a lot more variables to fill your list. To initialize your list with a number a default created values, simply write b = std::list<Bunny>(initialPopulation);. This will fill the list with initialPopulation bunnies, which should be some variable in your class.

In your end_turn method you should use a range-based-for loop instead of your while loop. That will make getting the begin iterator beforehand obsolete:

for (auto& bunny : b)
{
    bunny.increment_age();
    // ...
}

Use auto when it's appropriate. E.g. in std::list<Bunny>::iterator bunny = normal_bunnies.begin(); simply write auto bunny = normal_bunnies.begin(); No need to write something the compiler knows anyway. This way you can also change the container type without changing this part of the code.

Unecessary copies

In the following code, you're creating and copying too many objects:

    for (Bunny p : females)
    {
        Bunny new_bunny{p.get_color()};
        b.push_back(new_bunny);
    }

The use of the range-based-for is great, but you should use a Bunny constreference there, since you only need to read one value of it. Otherwise each female would be copied as the variable p. Furthermore you are creating a new Bunny object and copy that into your list. You should use the emplace_back method in this case, which will directly create the object in the list:

    for (auto const& p : females)
    {
        b.emplace_back(p.get_color());
    }

Game

Currently your game is just a bunch of methods. It should be a class that keepts track of its population itself, not by passing a list of bunnies. Now I can easily "cheat" the game, by manipulation the bunny list from outside. By keeping track of its own population you could also do things like running multiple games simultaniously, e.g. to compare experiments based on different starting conditions. This would eliminate the static count variables in your bunny class.

Seperation of concerns

You should seperate your logic from your output. E.g the Bunny shouldn't do any output. It's the games task to do that.

Update:

Use std::vector as a default

Currently you are using std::list as your container for bunnies. While that sounds reasonable, in C++ you should stick to std::vector unless you have a very good reason not to. Especially since you are randomly selecting bunnies from you list to manipulate them, a std::vector is far better than a std::list, since that can be done immediately, while for a list you have to loop all the way to that element.

Bug

As mentioned in the comments, the method transform_random_bunny doesn't do what you want. Through the method get_normal_bunnies you are creating a copy of your initial bunnies that a normal. You then only iterate and manipulate this copies list of bunnies, not the original. So your vampire bunnies are not successfull at all. So instead of returning a list of normal bunnies, return the indices of the normal bunnies. And then randomly select ab index out of that list and change that bunny:

void transform_random_bunny(std::vector<Bunny>&b)
{
    std::vector<size_t> normal;
    for (size_t i = 0; i < b.size(); ++i)
        if (!b[i].is_radioactive())
            normal.push_back(i);

    for (auto i = 0; i < Bunny::count_radioactive_bunnies && !normal.empty(); ++i)
    {
        auto pos = Random::integer(0, normal.size() - 1);
        b[normal[pos]].set_radioactive(true);
        std::cout << "transformed " << b[normal[pos]].get_name() << "\n";
        normal.erase(normal.begin() + pos);
    }
}

Note that I already change the container type to std::vector. Otherwise the random access through [] would not work and you'd have to use advance again (or save the iterators instead of indices).

Answer 2

Don't try to optimize without reason

I'm looking for feedback specifically on performance

Don't. You're writing code which:

• Involves a small amount of data - no more than 1,000 bunnies, each of them of them limited to, what, less than 32 bytes and that's if you make copies of the names rather using name indices.
• Involves a small number of iterations - in expectation anyway. Once you have a radioactive vampire bunny born your colony will be dead in, let's say, well under a 100 turns in expectation (back-of-the-envelope estimate); and assuming you don't run out of females (in which case the colony will die within 49 years), you have an extremely low chance of not getting a radioactive mutant within, say, 10 * 1/2% = 500 turns. So overall, very low probability of over 1,000 turns if I figure things right.
• You're using a linked list, so your performance is going to be rather weak anyway.

It makes no sense to optimize inherently-slow code which won't benefit from optimization anyway. Focus (in general, not just in this example) on writing clean, readable code, using standard libraries where relevant, sticking to the C++ core guidelines (those you agree with anyway) - and that's quite enough.

Only when you know with relative certainty that your code is now, or is going to be, slower than you need it to - then start investing in performance enhancement.

Don't use rand() - use the C++11 <random> facilities

Watch this: rand() considered harmful!. Convinced? Good. Instead of using rand(), use C++11's <random> facilities. Here's how:

https://stackoverflow.com/questions/32927722/how-to-use-random-to-replace-rand

Note you'll want an std::bernoulli_distribution for flipping coins.

Use meaningful names

Remember, code is written once (hopefully) and read many times. So it's not a reasonable idea to shorten names to spare yourself typing. Thus, for example, instead of:

void set_radioactive(bool b);

use

void set_radioactive(bool is_radioactive);

Avoid "misleading APIs"

Example: The Bunny method

public:
    void set_radioactive(bool is_radioactive);

misleads the reader: It suggests you can remove the radioactivity mutation, which you cannot. Something like:

void make_radioactive();

should suffice. Now, this too is not perfect, as it does not reflect the fact that only other radioactive bunnies can make bunnies radioactive (after construction), but let's not go there.

Use const members

Instead of having protected members with public getters, just make them public const members.

Don't keep game information inside the bunny class

The total and radioactive bunny counts are meaningful in the context of a game; keep them there - either as globals, as locals of main(), or as member variables of a Game class instance.

Don't "memoize" essentially-static data without good reason

Your Bunny class has a max_age member. Why? Can you have a different maximum age for every bunny? No, that depends solely on whether or not you're radioactive. So really,

int max_age() const { return is_radioactive() ? 50 : 10; }

it would only make sense to "memoize" the result of this function, and keep a max_age member, if computing it was relatively expensive and it was used frequently. The former certainly doesn't hold in our case.