Another Conway's Game of Life Simulation (With colored populations!)

This is my near-final version of Conway's Game of Life, with inherited colors using PDCurses. Any new spawned cells take on the most frequent color surrounding it when it spawns. This leads to single-colored populations roaming around, and causes interesting "battles" between 2 populations; which usually end in one taking over the other.

I already had one review of it, which increased its speed considerably, but now I'm looking for more general feedback about best practices, and feedback regarding the following:

• An alternative to the struct NeighborData. It seems overkill to use a struct like this just to return 2 pieces of data at once (The most prevalent color, and the number of neighbors).

• If there's a more efficient way of counting the colors than consumeColorFreqs

• Anywhere else I can squeeze more speed out of.

Population.h:

#ifndef POPULATION_H
#define POPULATION_H

#include 
#include 
#include 

#include "curses.h"

#define NCOLORS 16

typedef unsigned char Color;

struct NeighborData {
    unsigned int count = 0;
    Color color = COLOR_WHITE;
    NeighborData(unsigned int ct, Color cr);
};

class Population {
    //To hold the "finished" generation, and the generation
    // currently being constructed
    std::vector cells;
    std::vector newCells;

    int width = 0, height = 0;

public:
    Population(int newWidth, int newHeight);

    bool pointIsOccupied(int x, int y) const;

    void addPoint(int x, int y, Color color);
    void killPoint(int x, int y);

    Color getPointColor(int x, int y) const;

    NeighborData getNeighborData(int x, int y, int depth = 1) const;

    void decideLifeOf(int, int);

    int getIndexOf(int, int) const;

    void replacePopulation();

    Color consumeColorFrequencies(const Color colorFreqs[]) const;

};

Color randomColor(Color starting = 1);

#endif

Population.cpp:

```
#include "Population.h"

#include
#include
#include

#include "curses.h"

NeighborData::NeighborData(unsigned int ct, Co

I see a number of things that could probably help you improve your program.

Eliminate unused variables

In main(), argc and argv are unused. I'd recommend either eliminating them by changing the function to int main() or better yet, use them to pass in the required initialization variables for dimensions and seed value.

Use standard function where available

The code uses a custom strToInt function, but it would be better to simply use std::stoi.

Use const where practical

The values for maxX, maxY are set once and never changed which suggests that they should be const instead. I'd recommend eliminating inSeed, tempSeed, inX and inY and all of the associated clutter and instead using this:

if (argc < 3) {
    cout << "Usage: life xdim ydim seedvalue\n";
    return 0;
}
const int maxX = std::stoi(argv[1]);
const int maxY = std::stoi(argv[2]);
const int seed = std::stoi(argv[3]);

Have your constructor fully construct the object

The code currently has the equivalent to these lines:

World w(maxX, maxY);
w.randomizeCells(0.4, seed);

Instead, why not simply make a constructor that does the randomization?

World w(maxX, maxY, 0.4, seed);

Now the World object is fully instantiated and ready to use which should be the result of every constructor. One way to do that:

World::World(int xMax, int yMax, double chanceOfLife, int newSeed) :
    worldWidth(xMax), 
    worldHeight(yMax),
    pop(worldWidth, worldHeight)
{
    randomizeCells(chanceOfLife, newSeed);
}

Name parameters in constructors

At the moment, the constructor for the World object has this declaration inside the World.h header:

World(int, int);

Unfortunately, this is not sufficient for someone trying to use the World object. A very simple addition would be to simply name the parameters:

World(int xMax, int yMax);

This doesn't take the place of good documentation, but it's very useful for quick reference and costs you nearly nothing.

Never use an external parameter as a printf format string

The World::compileOutput() routine includes this line:

mvprintw(cY, cX, (pop.pointIsOccupied(cX, cY) ? disp.c_str() : " ") );

However disp is a std::string passed from outside the object. This is potentially dangerous because it could potentially be used for a format string attack. A better alternative would be this:

mvaddch(cY, cX, (pop.pointIsOccupied(cX, cY) ? disp : ' ') );

Now if you make the passed parameter a char, there is no longer any way to abuse it. Further, you avoid the overhead of having to create and destroy a string object every interation, which helps performance.

Avoid calling extra functions

In the World::compileOutput() routine, there there are calls to attron and attroff surrounding every output. However, these are not necessary. Instead, the could could look like this:

char c = pop.getPointColor(cX, cY);
init_pair(c, c, COLOR_BLACK);   //(Pair number, fore color, back color)
mvaddch(cY, cX, (pop.pointIsOccupied(cX, cY) ? disp|COLOR_PAIR(c) : ' ') );

By ORing the color value into the character value, you can avoid the two calls.

Use only required #includes

Your .h files are the interface for the class. For that reason, in those files, you should only include the #include files that are required for the interface. So in the case of World.h the file currently has the following includes:

#include 
#include 
#include 
#include 

#include "Population.h"

However, only Population.h is actually required for the interface. The others are required for the implementation but that is a detail that is not of concern to users of the interface, so those should be moved to the World.cpp file instead.

Use C++11 random number facilities

Consider this code in your World::randomizeCells() routine:

for (int y = 0; y < worldHeight; y++) {
    for (int x = 0; x < worldWidth; x++) {
        if ((rand() % int(1.0 / chanceOfLife)) == 0) {
            unsigned char color = randomColor();
            pop.addPoint(x, y, color);
        }
    }
}

What the code is attempting to do is to generate a boolean value distribution that is true with a probability of chanceOfLife. So if chanceOfLife has the value of 0.25 we would expect about 1 in 4 values to be true. That's called a Bernoulli distribution and C++11 has a much more direct and much more elegant way to achieve this.

std::random_device rd;
std::bernoulli_distribution alive(chanceOfLife);
for (int y = 0; y < worldHeight; y++) {
    for (int x = 0; x < worldWidth; x++) {
        if (alive(rd)) {
            pop.addPoint(x, y, randomColor());
        }
    }
}

You will need to #include to use this facility.

Rethink your interfaces

The World and Population objects are closely intertwined. It appears that the World is essentially responsible for displaying the data and that Population is responsible

StackExchange Code Review Q#88232, answer score: 6