0
\$\begingroup\$

I'm learning C++ so I thought I'd make game of life for practice and explore pointers, 1.5D arrays and stuff like that.

I have "finished" my implementation, but I have trouble with memory access. It seems like I'm stepping over the bounds somewhere and I simply can't figure out where. Everything looks fine to me.

Problem is most likely in the function called void World::follow that is checking surrounding cells if they are alive and than sets state for center cell for the next generation/tick depending of how many neighbors it has but I simply don't see it.

Here is my code:

source.cpp

#include "World.h"

int main() {
    World world = World(40, 20, 20);
    while (world.update())
        world.render();
}

Organism.h

#pragma once

class Organism {
private:
    bool cState, nState;
public:
    Organism(bool state);
    void perish();
    void revive();
    bool status();
    void update();
};

World.h

#pragma once
#include "Organism.h"

class World {
private:
    Organism** plain;
    int population;
    short width, height;
public:
    World(short width, short height, short chance);
    ~World();
    bool update();
    void render();
    void follow(short x, short y);
};

Organism.cpp

#include "Organism.h"

Organism::Organism(bool state) {
    this->cState = state;
}

void Organism::perish() {
    this->nState = false;
}

void Organism::revive() {
    this->nState = true;
}

bool Organism::status() {
    return this->cState;
}

void Organism::update() {
    this->cState = this->nState;
}

World.cpp

#include "World.h"

#include <time.h>
#include <stdlib.h>
#include <iostream>
#include <Windows.h>

using namespace std;

World::World(short width, short height, short chance) {
    this->population = 0;
    this->width = width; this->height = height;

    this->plain = new Organism*[width*height];

    bool state;
    srand(time(nullptr));
    for (int i = 0; i < (width*height); i++) {
        state = rand() % 100 < chance;
        this->plain[i] = new Organism(state);
    }

    for (short x = 0; x < this->width; x++)
        for (short y = 0; y < this->height; y++)
            this->follow(x, y);
}

bool World::update() {
    this->population = 0;

    for (int i = 0; i < (width*height); i++) {
        this->plain[i]->update();
        if (this->plain[i]->status())
            this->population++;
    }

    for (short x = 0; x < this->width; x++)
        for (short y = 0; y < this->height; y++)
            this->follow(x,y);

    return this->population > 0;
}

void World::follow(short x, short y) {
    bool alive;
    short pals = 0;
    Organism* current = this->plain[x*this->width + y];

    short sx = (x - 1 < 0) ? x : x - 1;
    short sy = (y - 1 < 0) ? y : y - 1;
    short ex = (x + 1 == this->width - 1) ? x : x + 1;
    short ey = (y + 1 == this->height - 1) ? y : y + 1;

    for (short row = sx; row < ex; row++) {
        for (short col = sy; col < ey; col++) {
            alive = this->plain[row*this->width + col]->status();
            if (row != x && col != y && alive) pals++;
        }
    }

    if (!current->status() && pals == 3)
        current->revive();
    else if (current->status() && pals != 2 && pals != 3)
        current->perish();
}

void World::render() {
    Sleep(500);
    system("cls");
    for (short x = 0; x < this->width; x++) {
        for (short y = 0; y < this->height; y++) {
            cout << (this->plain[x*this->width + y]->status()) ? " " : "x";
        }
        cout << endl;
    }
}

World::~World() {
    for (int i = this->width * this->height; i >= 0; i--)
        delete this->plain[i];
    delete[] this->plain;
}
\$\endgroup\$

closed as off-topic by Phrancis, Jamal Mar 28 '15 at 20:04

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Phrancis, Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Also, as a side question... did I do my destructor right? \$\endgroup\$ – Reygoch Mar 28 '15 at 19:28
  • \$\begingroup\$ Welcome to Code Review! Here we only review code that works as intended. If you can narrow down your problem to something specific, it may be a good questions for Stack Overflow. Once your code works as intended, we will be happy to review it and help you make it better. \$\endgroup\$ – Phrancis Mar 28 '15 at 19:31
  • \$\begingroup\$ You got your index calculations the wrong way around. x*width + y is wrong. It should be x + width*y. Note: For every row you go down you must traverse width cells. \$\endgroup\$ – Martin York Mar 28 '15 at 20:29
  • \$\begingroup\$ Which leads to another point that calculation should be done in its own functions. That way you fix it once and it applies everywhere. \$\endgroup\$ – Martin York Mar 28 '15 at 20:31
2
\$\begingroup\$

Stop doing manually memory management (this is not C).

Organism** plain;

There is no reason to have an array of pointers. You could just have an array of object (thus you don't need to manage the individual cells).

Organism*  plain;
....
plain = new Organism[width * height]; // Allocates and calls default constructor.
                                      // You will still need to loop over
                                      // them to reset them to an initial good
                                      // state (but you will not have) to
                                      // manage the memory for individual cells.

Because the cells are no longer pointers you will need to replace -> with . (a dot). But other than than no difference.

Part 2 of memory management

Take this a step further. You don't need to manually manage the memory for the array of plain. We have classes that do that for you.

std::vector<Organism>  plain;        // initially this holds zero elements
...

plain.reserve(width * height);       // Let the object know you are going to
                                     // add a lot of cells so it can pre-allocate space.

for (int i = 0; i < (width * height); i++) {
    state = rand() % 100 < chance;
    plain.emplace_back(state);       // add a new element to the end.
    // If you are using C++03 you may not have emplace_back()
    // So use the regular push_back instead (it does the same thing).
    //    plain.push_back(Organism(state));
}

You can access plain (a std::vector<>) with operator [] like you would an array. So no other code needs to change (you just need any new or delete calls). So you have no memory management calls and thus no headaches with leaks.

Now a nice bit is you can replace plain[x] with plain.at(x) and it checks to make sure you access to the array is in bounds (and will throw an exception if not). This may help you track down your error.

Initialize srand() one

You should only initializre the random number generator once in a program.

srand(time(nullptr));

So it is best to do this in main(). In your application you are doing this once for each game object. This may not seem like a big thing. But of your code get's re-used by another application that does not realize you are doing this. You mess up other people's code.

You should also note. This old random number generator is considered pretty bad. You should probably start looking at the new ones provided by C++11. They are much better and can be used in ways that don't interact (so you don't screw over other people's random numbers).

Move Platform specific code to its own function.

Sleep(500);
system("cls");

You should probably put this in its own function. So when people come to port your code to another platform they just need to provide an alternative implementation to a couple of specific functions and the rest of the code can stay the same.

\$\endgroup\$
  • \$\begingroup\$ I actually wanted to skip default constructor (because it annoys me when I have to reset everything, why have constructor at all than?) and go into memory management my self for the sake of learning how stuff work in C++. \$\endgroup\$ – Reygoch Mar 28 '15 at 19:48
  • \$\begingroup\$ Didn't know I can reserve memory in vector in advance. Thanks for that tip! \$\endgroup\$ – Reygoch Mar 28 '15 at 19:49
  • \$\begingroup\$ @Reygoch: The first step is just to get you passed using an array of pointers. Step 2 using the vector allows you to again use the constructor correctly. I just broke it into two steps. \$\endgroup\$ – Martin York Mar 28 '15 at 19:51
  • \$\begingroup\$ @Reygoch: With memory (resource) management. The most important rule is Separation of Concerns. An class should be designed for either Business Logic or Resource Management. A class should not do both. If you want to do the memory management then break into into a separate class to be done explicitly (but it will look like std::vector). Your business logic should not worry about resource management as it complicates the class unnecessarily and distracts you from what you should be doing. \$\endgroup\$ – Martin York Mar 28 '15 at 19:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.