11
\$\begingroup\$

I am looking for a review on one of my homework assignments for this semester.

This homework has already been submitted and graded and my final has already been submitted so there is no cheating or conflict of interest in my review request!

I would love some advice on how to better manage my class interactions and how to better encapsulate data. The specific area I struggle with is when using parent classes and giving access to only protected assets from child classes. My professor has mentioned on several occasions that it would be much better to keep data private and give access through functions, but how would I initialize those member variables when instantiating an instance of a child class that has private members in the parent class?

I understand the idea of encapsulation is to protect data that shouldn't be manipulated by outside programmers or irrelevant classes. I was the sole developer on this project so I understand in this specific example encapsulation may not be paramount, but on a larger project with multiple engineers it would certainly be relevant.

Includes

#include "stdafx.h"
#include <iostream>
#include <ctime>
#include <vector>

using namespace std;

** Note the coordinates struct just contains an integer xCoordinate and an integer yCoordinate

main

int main()

{
    //Create environment object containing 
    environment antDoodlebugSimulation;

    antDoodlebugSimulation.InitializeSimulation();

    return 0;
}

Environment

class environment
{
    //friends of environment
    friend class organism;
    friend class doodlebug;
    friend class ant;

private:
    organism * environmentBoard[20][20];
    void CreateStartPopulation();
    int GenerateRandomStartingLocations(int min, int max);
    void OutputCurrentEnvironment();
    void DoodlebugsAct();
    void AntsAct();
    void ResetCritterTimeStep();

public:
    //constructor
    environment();

    //deconstructor
    ~environment();

    //public member functions
    void InitializeSimulation();

};

environment::environment()
{
    //Initialize environmentBoard array for entirely empty...all nullptr
    for (int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
            this->environmentBoard[xCounter][yCounter] = nullptr;

    //Add ants and doodlebugs into game environment
    CreateStartPopulation();
}

void environment::OutputCurrentEnvironment()
{
    //Outputs current environment to the screen
    for (int yCounter = 0; yCounter < 20; yCounter++)
    {
        for (int xCounter = 0; xCounter < 20; xCounter++)
            if (this->environmentBoard[xCounter][yCounter] == nullptr)
                cout << '-';
            else
                cout << *environmentBoard[xCounter][yCounter];

        cout << endl;
    }
}

void environment::CreateStartPopulation()
{
    //Populates game board for start of simulation
    coordinates newLocation;

    //Fill 5 doodlebugs
    for (int doodleCounter = 0; doodleCounter < 5; doodleCounter++)
    {
        newLocation.xCoordinate = GenerateRandomStartingLocations(0, 19);
        newLocation.yCoordinate = GenerateRandomStartingLocations(0, 19);

        //Incase something already placed
        while (this->environmentBoard[newLocation.xCoordinate][newLocation.yCoordinate] != nullptr)
        {
            newLocation.xCoordinate = GenerateRandomStartingLocations(0, 19);
            newLocation.yCoordinate = GenerateRandomStartingLocations(0, 19);
        }

        this->environmentBoard[newLocation.xCoordinate][newLocation.yCoordinate] = new doodlebug(newLocation.xCoordinate, newLocation.yCoordinate, this);
    }
    for (int antCounter = 0; antCounter < 100; antCounter++)
    {

        newLocation.xCoordinate = GenerateRandomStartingLocations(0, 19);
        newLocation.yCoordinate = GenerateRandomStartingLocations(0, 19);

        //Incase something already placed
        while (this->environmentBoard[newLocation.xCoordinate][newLocation.yCoordinate] != nullptr)
        {
            newLocation.xCoordinate = GenerateRandomStartingLocations(0, 19);
            newLocation.yCoordinate = GenerateRandomStartingLocations(0, 19);
        }

        this->environmentBoard[newLocation.xCoordinate][newLocation.yCoordinate] = new ant(newLocation.xCoordinate, newLocation.yCoordinate, this);
    }
}

int environment::GenerateRandomStartingLocations(int min, int max)
{
    static bool firstIteration = true;
    if (firstIteration)
    {
        //Seed once
        srand(time(NULL));
        firstIteration = false;
    }
    //Generate random number between range
    return rand() % (max - min + 1) + min;
}

void environment::AntsAct()
{

    //Iterate over environment
    for(int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (this->environmentBoard[xCounter][yCounter] != nullptr)
            {
                if (this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o' && !(this->environmentBoard[xCounter][yCounter]->GetMoveAlternatorStatus()))
                {   
                    //Toggle moveAlternator
                    this->environmentBoard[xCounter][yCounter]->ToggleMoveAlternator();

                    //Move ant....Move function must return coordinate otherwise we attempt to increment/breed null pointer after movement
                    this->environmentBoard[xCounter][yCounter]->Move();
                }
            }
        }

    //Iterate over Ants again and breed
    for(int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (this->environmentBoard[xCounter][yCounter] != nullptr)
            {
                if (this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o')
                {
                    //Increment Ant Breeding cycle
                    this->environmentBoard[xCounter][yCounter]->IncrementBreedingCycle();

                    //Check if ready to breed
                    if (this->environmentBoard[xCounter][yCounter]->TimeToBreed())
                    {
                        this->environmentBoard[xCounter][yCounter]->Breed();
                    }
                }
            }
        }
}

void environment::DoodlebugsAct()
{
    //Move and eat if ant is found
    for (int yCounter = 0; yCounter < 20; yCounter++)
    {
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            //Not null pointer...a doodlebug....and has not already moved
            if (this->environmentBoard[xCounter][yCounter] != nullptr && this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'X' && !this->environmentBoard[xCounter][yCounter]->GetMoveAlternatorStatus())
            {
                this->environmentBoard[xCounter][yCounter]->ToggleMoveAlternator();
                this->environmentBoard[xCounter][yCounter]->Move();
            }
        }
    }
    //Breed and then check death cycle
    for (int yCounter = 0; yCounter < 20; yCounter++)
    {
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (this->environmentBoard[xCounter][yCounter] != nullptr && this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'X')
            {
                //Increment breeding step
                this->environmentBoard[xCounter][yCounter]->IncrementBreedingCycle();

                //Check if ready to breed
                if (this->environmentBoard[xCounter][yCounter]->TimeToBreed())
                    this->environmentBoard[xCounter][yCounter]->Breed();

                //Check if dying
                if (this->environmentBoard[xCounter][yCounter]->Death())
                    this->environmentBoard[xCounter][yCounter]->Die();
            }
        }
    }
}

void environment::ResetCritterTimeStep()
{
    //Reset all critter time steps at the end of iteration
    for (int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
            if (this->environmentBoard[xCounter][yCounter] != nullptr && (this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o' || this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'X'))
                this->environmentBoard[xCounter][yCounter]->ToggleMoveAlternator();
}

void environment::InitializeSimulation()
{
    int numOfants, numOfDoodleBugs;

    OutputCurrentEnvironment();
    cout << "Simulation initialized starting with 5 doodle bugs and 100 ants.  Please begin simulation by pressing the enter key\n";

    while(cin.get() == '\n')
    {
        //Manage simulation
        numOfants = 0;
        numOfDoodleBugs = 0;

        //Call doodlebugs to act
        DoodlebugsAct();

        //Call ants to act
        AntsAct();

        //Reset all critter time steps
        ResetCritterTimeStep();

        //output graphical environment status
        OutputCurrentEnvironment();

        //For easy visual of fluctuations between predator and prey populations
        for (int yCounter = 0; yCounter < 20; yCounter++)
            for (int xCounter = 0; xCounter < 20; xCounter++)
                if (this->environmentBoard[xCounter][yCounter] != nullptr)
                    if (this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o')
                        numOfants++;
                    else
                        numOfDoodleBugs++;

        cout << "Ants: " << numOfants << endl;
        cout << "Doodlebugs: " << numOfDoodleBugs << endl;

        cout << "To continue simulation please press enter; otherwise enter -1: ";
    }
}

environment::~environment()
{
    //Clean up memory for object and repoint all dangling pointers
    for(int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (this->environmentBoard[xCounter][yCounter] != nullptr)
            {
                delete this->environmentBoard[xCounter][yCounter];
                this->environmentBoard[xCounter][yCounter] = nullptr;
            }
        }
}

Organism

class organism
{
    //Organism parent class
    private:
        bool moveAlternator = false;

    protected:
        environment * environmentPointer;   
        int breedingCycle = 0;
        char identifierTag;
        coordinates location;

    public:
        //Constructor
        organism() {location.xCoordinate = 0; location.yCoordinate = 0;};
        //deconstructor
        virtual ~organism() {};
        char GetIdentifierTag() const { return this->identifierTag; };
        void IncrementBreedingCycle() { breedingCycle += 1; };
        coordinates GetLocation() const { return this->location; };
        virtual void Move() = 0;
        void RandomDirectionalMovement(int direction);
        virtual bool TimeToBreed() = 0;
        void Die();
        virtual bool Death() =0;
        void Breed();
        void ResetBreedingCycle() { this->breedingCycle = 0; };
        void ToggleMoveAlternator();
        bool GetMoveAlternatorStatus() const { return moveAlternator; };
        void GoNorth() { this->location.yCoordinate -= 1; };
        void GoEast() { this->location.xCoordinate += 1; };
        void GoSouth() { this->location.yCoordinate += 1; };
        void GoWest() { this->location.xCoordinate -= 1; };

        friend ostream &operator<<(ostream &output, const organism &currentOrg) { output << currentOrg.identifierTag; return output; };
};

void organism::ToggleMoveAlternator()
{
    if (this->moveAlternator == false)
        this->moveAlternator = true;
    else
        this->moveAlternator = false;
}

void organism::Breed()
{
    //Breeds new 
    //Generate direction to breed in...if empty..breed
    int newDirection = this->environmentPointer->GenerateRandomStartingLocations(1, 4);
    bool breedSuccesful = false;

    //North
    if (newDirection == 1)
    {
        if (this->location.yCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] == nullptr)
        {
            if (this->GetIdentifierTag() == 'o')//Breed new ant
                this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] = new ant(this->location.xCoordinate, this->location.yCoordinate - 1, this->environmentPointer);
            else if (this->GetIdentifierTag() == 'X')//Breed new doodlebug
                this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] = new doodlebug(this->location.xCoordinate, this->location.yCoordinate - 1, this->environmentPointer);

            breedSuccesful = true;
        }
    }
    //East
    else if (newDirection == 2)
    {
        if (this->location.xCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate] == nullptr)
        {

            if(this->GetIdentifierTag() == 'o')//Breed new ant
                this->environmentPointer->environmentBoard[this->location.xCoordinate+1][this->location.yCoordinate] = new ant(this->location.xCoordinate+1, this->location.yCoordinate, this->environmentPointer);
            else if (this->GetIdentifierTag() == 'X')//Breed new doodlebug
                this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate] = new doodlebug(this->location.xCoordinate + 1, this->location.yCoordinate, this->environmentPointer);

            breedSuccesful = true;
        }
    }
    //South
    else if (newDirection == 3)
    {
        if (this->location.yCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate + 1] == nullptr)
        {
            if (this->GetIdentifierTag() == 'o')//Breed new ant
                this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate+1] = new ant(this->location.xCoordinate, this->location.yCoordinate+1, this->environmentPointer);
            else if (this->GetIdentifierTag() == 'X')//Breed new doodlebug
                this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate+1] = new doodlebug(this->location.xCoordinate, this->location.yCoordinate+1, this->environmentPointer);

            breedSuccesful = true;
        }
    }
    //West
    else if (newDirection == 4)
    {
        if (this->location.xCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate - 1][this->location.yCoordinate] == nullptr)
        {
            if (this->GetIdentifierTag() == 'o')//Breed new ant
                this->environmentPointer->environmentBoard[this->location.xCoordinate-1][this->location.yCoordinate] = new ant(this->location.xCoordinate-1, this->location.yCoordinate, this->environmentPointer);
            else if (this->GetIdentifierTag() == 'X')//Breed new doodlebug
                this->environmentPointer->environmentBoard[this->location.xCoordinate-1][this->location.yCoordinate] = new doodlebug(this->location.xCoordinate-1, this->location.yCoordinate, this->environmentPointer);

            breedSuccesful = true;
        }
    }

    if (breedSuccesful)
        this->ResetBreedingCycle();
}
void organism::RandomDirectionalMovement(int direction)
{
    //North
    if (direction == 1)
    {
        if (this->location.yCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] == nullptr)
        {
            //Empty space found and valid location within stated environment
            //Point new location to current organism object
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] = this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate];

            //repoint old location to null
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate] = nullptr;

            //update ant location
            this->GoNorth();
        }
    }
    //East
    else if (direction == 2)
    {
        if (this->location.xCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate] == nullptr)
        {
            //Empty space found and valid location with stated environment
            //Point new location to current ant object
            this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate] = this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate];

            //Repoint old location to null
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate] = nullptr;

            //update ant location
            this->GoEast();
        }
    }
    //South
    else if (direction == 3)
    {
        if (this->location.yCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate + 1] == nullptr)
        {
            //Empty space found and valid location with stated environment
            //Point new location to current ant object
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate + 1] = this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate];

            //Repoint old location to null
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate] = nullptr;

            //update organism location
            this->GoSouth();
        }
    }
    //West
    else if (direction == 4)
    {
        if (this->location.xCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate - 1][this->location.yCoordinate] == nullptr)
        {
            //Empty space found and valid location with stated environment
            //Point new location to current organism object
            this->environmentPointer->environmentBoard[this->location.xCoordinate - 1][this->location.yCoordinate] = this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate];

            //Repoint old location to null
            this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate] = nullptr;

            //update organism location
            this->GoWest();
        }
    }
}

void organism::Die()
{
    //repoint location in environment to null
    this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate] = nullptr;

    //delete
    delete this;

doodlebug

class doodlebug : public organism
{
    //Doodlebug child class

    private:
        int starveCounter = 0;

    public:
        //Constructor
        doodlebug(int xLocation, int yLocation, environment * worldPointer) { identifierTag = 'X'; location.xCoordinate = xLocation; location.yCoordinate = yLocation; environmentPointer = worldPointer; };
        //Deconstructor
        ~doodlebug() {};

        bool TimeToBreed();
        void Move();
        coordinates SearchForAnts();
        void IncrementStarveCounter() { this->starveCounter += 1; };
        void ResetStarveCounter() { this->starveCounter = 0; };
        bool Death() { if (this->starveCounter == 3) return true; else return false; };

};

coordinates doodlebug::SearchForAnts()
{
    coordinates antFound;
    antFound.xCoordinate = -1;
    antFound.yCoordinate = -1;

    //store checked direction
    int directionCheck;
    vector<int> storeDirections;

    directionCheck = this->environmentPointer->GenerateRandomStartingLocations(1, 4);

    //randomize first search direction and store already searched directions
    while (storeDirections.size() < 4 && antFound.xCoordinate == -1 && antFound.yCoordinate == -1)
    {
        //Search North for ant...verify not off the board...verify not null pointer...check if ant
        if (directionCheck == 1 && this->location.yCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1] != nullptr)
        {
            if (this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate - 1]->GetIdentifierTag() == 'o')
            {
                antFound.xCoordinate = this->location.xCoordinate;
                antFound.yCoordinate = this->location.yCoordinate - 1;
            }
        }
        //Search East for Ant....
        else if (directionCheck == 2 && this->location.xCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate] != nullptr)
        {
            if (this->environmentPointer->environmentBoard[this->location.xCoordinate + 1][this->location.yCoordinate]->GetIdentifierTag() == 'o')
            {
                antFound.xCoordinate = this->location.xCoordinate + 1;
                antFound.yCoordinate = this->location.yCoordinate;
            }
        }
        //Search South for Ant....
        else if (directionCheck == 3 && this->location.yCoordinate + 1 < 20 && this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate + 1] != nullptr)
        {
            if (this->environmentPointer->environmentBoard[this->location.xCoordinate][this->location.yCoordinate + 1]->GetIdentifierTag() == 'o')
            {
                antFound.xCoordinate = this->location.xCoordinate;
                antFound.yCoordinate = this->location.yCoordinate + 1;
            }
        }
        //Search west for Ant...
        else if (directionCheck == 4 && this->location.xCoordinate - 1 > -1 && this->environmentPointer->environmentBoard[this->location.xCoordinate - 1][this->location.yCoordinate] != nullptr)
        {
            if (this->environmentPointer->environmentBoard[this->location.xCoordinate - 1][this->location.yCoordinate]->GetIdentifierTag() == 'o')
            {
                antFound.xCoordinate = this->location.xCoordinate - 1;
                antFound.yCoordinate = this->location.yCoordinate;
            }
        }

        //Store direction check to increase size of stored directions....guarantees 4
        storeDirections.push_back(directionCheck);
        if (directionCheck < 4)
        {
            directionCheck += 1;
        }
        else
            directionCheck = 1;
    }
    return antFound;
}

Ant

class ant : public organism
{
    //ant child class

    private:


    public:
        //Constructor
        ant(int xLocation, int yLocation, environment * thisWorld) { identifierTag = 'o'; location.xCoordinate = xLocation; location.yCoordinate = yLocation; environmentPointer = thisWorld; };

        //Deconstructor
        ~ant() {};

        void Move();
        bool TimeToBreed();
        bool Death() { return false; };

};
\$\endgroup\$
  • \$\begingroup\$ Does this compile for you? It doesn't compile on my system Mac OSX El Capitan, Xcode 8.2.3. The code is also missing the includes and using statements (using std;). \$\endgroup\$ – pacmaninbw May 16 '17 at 16:39
  • \$\begingroup\$ #include "stdafx.h" #include <iostream> #include <ctime> #include <vector> using namespace std; \$\endgroup\$ – StormsEdge May 16 '17 at 16:58
  • \$\begingroup\$ i'll add that up top. Apologies \$\endgroup\$ – StormsEdge May 16 '17 at 16:58
  • \$\begingroup\$ and yes, it compiles. \$\endgroup\$ – StormsEdge May 16 '17 at 16:59
8
\$\begingroup\$

I see some things that may help you improve your program.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid.

Isolate platform-specific code

If you must have stdafx.h, consider wrapping it so that the code is portable:

#ifdef WINDOWS
#include "stdafx.h"
#endif

Separate interface from implementation

The interface for the class is typically separated into a .h file and the implementation is in the .cpp file. The advantage to doing things this way is that it tends to help the programmer enforce good class design. That leads directly to the next suggestion.

Rethink your class hierarchy

It's clear that when you wrote this, all of the code was in a single file. That's not necessarily wrong, but it can lead to poor class design, and that is indeed the case here. The way it is right now, every class needs to know about every other class. Even the organism class needs to know about the ant and doodlebug classes derive from it! This is surely a mistake. Instead, I'd recommend that you more clearly isolate classes. For example, the environment class might need to know about the organism class and clearly the ant and doodlebug classes must know about their base organism type, but that should be it. Some of the suggestions below more clearly spell out what's required to make that happen.

Fix the bug

The SearchForAnts() function is not currently called anywhere which, if you'll excuse the pun, looks like a bug to me.

Don't abuse friend

The environment class has the following three lines:

//friends of environment
friend class organism;
friend class doodlebug;
friend class ant;

However, none of those should really be needed. Instead, it would be better to simply forward declare the organism class and use only public interfaces to it.

Use inheritance effectively

The environment has a DoodlebugsAct() and AntsAct() routine, but that doesn't really make a lot of sense. If we think of the physical analog that this is trying to simulate, does the environment drive the actions of individual critters or does each critter act according to its own internal drives and a view of only a small subset of the environment that it can observe? I'd suggest it's more likely to be the latter, and that suggests that the organism class should probably have a function named act() that each derived class would implement and to which a vector of just the neighboring cells could be passed.

Prefer modern initializers for constructors

Constructors could use the more modern initializer style rather than the old style you're currently using. Instead of this:

ant(int xLocation, int yLocation, environment * thisWorld) { identifierTag = 'o'; location.xCoordinate = xLocation; location.yCoordinate = yLocation; environmentPointer = thisWorld; };

Write this:

ant(int xLocation, int yLocation, environment * thisWorld) : 
    identifierTag{'o'},
    location{xLocation, yLocation},
    environmentPointer{thisWorld}
{}

Note that this uses the C++11 initializer style which provides an unambiguous and consistent syntax as well as cleaner looking code.

Consider using a better random number generator

You are currently using

return rand() % (max - min + 1) + min;

There are two problems with this approach. One is that the low order bits of the random number generator are not particularly random, so neither are these random numbers. On my machine, there's a slight but measurable bias toward 0 with that. The second problem is that it's not thread safe because rand stores hidden state. A better solution, if your compiler and library supports it, would be to use the C++11 `std::uniform_int_distribution. It looks complex, but it's actually pretty easy to use.

Use a more appropriate data structure

The current environment is a 20x20 grid which contains either pointers to organisms or are nullptr. In most instances in which the structure is used, it would be easier to use as a simple linear array instead. It's also possible to have every pointer be a real pointer, and then the default actions (possibly having no effect) could be instantiated in the base organism class.

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:

[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:

If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

\$\endgroup\$
  • \$\begingroup\$ Out of curiosity, does using namespace std; ever have a proper use? \$\endgroup\$ – Conor O'Brien May 17 '17 at 4:05
  • \$\begingroup\$ @CᴏɴᴏʀO'Bʀɪᴇɴ: Good question. It can be used for a short single-file program that's not intended to be part of a library (e.g. won't ever be turned into a header file). It can also be usefully employed inside a function where the namespace pollution is limited to that one function. In my own programs, I almost never use it. \$\endgroup\$ – Edward May 17 '17 at 10:26
  • \$\begingroup\$ @Edward, it is also useful in a place where ADL on every function call is needed, and there are many function names. I haven't stumbled upon that yet, though std::begin()/end() and the family sometimes make it worth it. \$\endgroup\$ – Incomputable Apr 6 '18 at 2:03
9
\$\begingroup\$

Do not use namespace std;

This is a bad habit, that might bring you into trouble. You might want to search for it.

Avoid magic numbers

For example your board size is 20. And all over the code there is a 20 sprinkled in. What happens when you change the board size and forget one of those locations. Better create a member variable bordSize of the environement class and use that throughout the code. This makes it much more readable.

Same goes for stuff like this

//North
if (direction == 1)

Create enumerations like

enum direction {
    NORTH,
    SOUTH,
    EAST,
    WEST
};

Then you can do

if (direction == NORTH)

Use magic numbers consistently

You have 2 magic numbers in your code 19 and 20. But wait, they are the same. The difference is, that you GenerateRandomStartingLocations() is inclusive. Again, there is no reason, why this function should take arguments at all. create a member variable and use that inside of GenerateRandomStartingLocations()

Avoid using this-> inside a class

this-> is only necessary if you have name collisions. It is always better to simply not have name collisions rather than using this->

Use consistent braces

void environment::OutputCurrentEnvironment()
{
    //Outputs current environment to the screen
    for (int yCounter = 0; yCounter < 20; yCounter++)
    {
        for (int xCounter = 0; xCounter < 20; xCounter++)
            if (this->environmentBoard[xCounter][yCounter] == nullptr)
                cout << '-';
            else
                cout << *environmentBoard[xCounter][yCounter];

        cout << endl;
    }
}

The second for loop has no braclet and whats wore it holds a single line statement that goes over 4 lines. This is bound to introduce bugs in the long run. Also in such simple if else constructs you should rather use the ternary operator

void environment::OutputCurrentEnvironment()
{
    //Outputs current environment to the screen
    for (int yCounter = 0; yCounter < boardSize; yCounter++)
    {
        for (int xCounter = 0; xCounter < boardSize; xCounter++) 
        {
            std::cout << environmentBoard[xCounter][yCounter] == nullptr ? '-' : *environmentBoard[xCounter][yCounter] << "\n";
        }
    }
}

Avoid flushing cout

std::endl flushes the stream which is nearly always unneeded, so use a simple new line "\n"

Avoid meaningless comments

The above comment //Outputs current environment to the screen doesnt add anything to the code. So just omit it.

Avoid endless lines

Readability is key. Whenever you have to scroll left or right your lines are too long. Most open source project go for 80 characters which is quite sensible.

Use base class constructors when appropriate

You can defer a derived class constructor to the base class constructor https://stackoverflow.com/questions/6923722/how-do-i-call-the-base-class-constructor

Simplify if/else with boolean return

You have some if condition return true else false You can simpify that to return condition or return !condition

Same goes for assignments

if (this->moveAlternator == false)
    this->moveAlternator = true;
else
    this->moveAlternator = false;

Is equivalent to

moveAlternator = !moveAlternator;

You might want to use lambda functions

A lot of your code is incredibly repetitive. For example these lines appear extremely often

for (int yCounter = 0; yCounter < boardSize; yCounter++)
{
    for (int xCounter = 0; xCounter < boardSize; xCounter++)
    {

Afterwards, what you do inside the code is mostly small and well contained. Here you could define lambda functions and pass them as arguments to a function that just iterates over the loops and then calls the function https://stackoverflow.com/questions/3203305/write-a-function-that-accepts-a-lambda-expression-as-argument

https://stackoverflow.com/questions/8109571/lambda-as-function-parameter

\$\endgroup\$
5
\$\begingroup\$

You have obviously worked long and hard on this question.

When a class inherits from another class the constructor of the base class is implicity called by the constructor for the subclass. To initialize the private base class variables put the initialization of the variables in the base class constructor. To change the private variables of the base class the base class should provide protected accessor functions. An example, the boolean variable moveAlternator could have a settor function as well as a gettor function:

void SetMoveAltertor(bool NewValue)
{
    moveAlternator = NewValue;
}

or there could be a toggle function

void ToggleMoveAlternator()
{
    moveAlternator = (moveAlternator == true) ? false : true;
}

The Use of the this Pointer in C++

Unlike some languages such as PHP the this pointer isn't necessary to reference member functions or member variables. so code like this in environment::AntsAct()

    //Iterate over environment
    for(int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (this->environmentBoard[xCounter][yCounter] != nullptr)
            {
                if (this->environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o' && !(this->environmentBoard[xCounter][yCounter]->GetMoveAlternatorStatus()))
                {   
                    //Toggle moveAlternator
                    this->environmentBoard[xCounter][yCounter]->ToggleMoveAlternator();

                    //Move ant....Move function must return coordinate otherwise we attempt to increment/breed null pointer after movement
                    this->environmentBoard[xCounter][yCounter]->Move();
                }
            }
        }

can be more simply written as

    //Iterate over environment
    for(int yCounter = 0; yCounter < 20; yCounter++)
        for (int xCounter = 0; xCounter < 20; xCounter++)
        {
            if (environmentBoard[xCounter][yCounter] != nullptr)
            {
                if (environmentBoard[xCounter][yCounter]->GetIdentifierTag() == 'o' && !(environmentBoard[xCounter][yCounter]->GetMoveAlternatorStatus()))
                {   
                    //Toggle moveAlternator
                    environmentBoard[xCounter][yCounter]->ToggleMoveAlternator();

                    //Move ant....Move function must return coordinate otherwise we attempt to increment/breed null pointer after movement
                    environmentBoard[xCounter][yCounter]->Move();
                }
            }
        }

See this stackoverflow.com question for a discussion of when to use this->.

Initialization

The organism constructor could be written in organism.cpp as

organism::organism()
: moveAlternator{false}, location{0,0}, breedingCycle{0}
{
}

to initialize all of the varables in the organism class.

\$\endgroup\$
4
\$\begingroup\$

Constructors

Both your ant and doodlebug use the same initialization, so write the logic into your base class and simply redirect the constructor. Since you already have a coordinates class use it the constructor instead of decoupled parameters. Always try to Express ideas directly in code.

organism::organism(coordinates const& coords, environment* env)
    : location(coords)
    , environmentPointer(env)
{}

ant(coordinates const& coords, environment* env)
    : organism(coords, env)
{
    identifierTag = 'o';
}

but how would I initialize those member variables when instantiating an instance of a child class that has private members in the parent class?

This also answers your question. By forwarding to the base class constructor, the initialization its members is done there.

Accessing the environment

Many of your methods in organism and its sub classes access the underlying array of environment directly. I guess this is what your professor was talking about. It would be better to hide the implementation details of environment to the organism an provide methods to manipulate it. The organism doesn't care about the environment internals. All it wants to do is to check whatever is on some location and maybe move there. Also within all those methods you have a HUGE amount an redundant code. Another principle to follow is Don't repeat yourself. Define the logic to check what is on a location within the environment class. And the logic to get an adjacent location could be placed within the coordinates class, or also in the environment. This way your code would be shorter, clearer and less coupled.

void organism::RandomDirectionalMovement(int direction)
{
    auto newLocation = location.GetAdjacent(direction);
    if (environmentPointer->Move(this, newLocation))
        location = newLocation;
}

coordinates GetAdjacent(int direction) const
{
    switch (direction)
    {
        case 1:
            return { xCoordinate, yCoordinate - 1 };
        case 2:
            return { xCoordinate + 1, yCoordinate };
        case 3:
            return { xCoordinate, yCoordinate + 1 };
        case 4:
            return { xCoordinate - 1, yCoordinate };
    }
}

bool environment::IsValid(coordinates const& location) const
{
    return location.xCoordinate >= 0 && location.xCoordinate < 20
           && location.yCoordinate >= 0 && location.yCoordinate < 20;
}

bool environment::IsFree(coordinates const& location) const
{
    return IsValid(location) && environmentBoard[location.xCoordinate][location.yCoordinate] == nullptr;
}

bool environment::Move(organism* organism, coordinates const& location)
{
    if (organism && IsFree(location))
    {
        auto curLocation = organism->GetLocation();

        environmentBoard[curLocation.xCoordinate][curLocation.yCoordinate] = nullptr;
        environmentBoard[location.xCoordinate][location.yCoordinate] = organism;
        return true;
    }
    return false;
}

The same applies for other methods as well.

Don't use magic numbers

In your environment class you hard-coded its size. If you want to change its size for another experiment you have to change a whole lot of code and probably forget or mix up some, especially if you don't want to use a square board. So make them a constexpr and refer to their name in your code.

class environment
{
public:
    static constexpr int width = 20;
    static constexpr int height = 20;
private:
    organism* environmentBoard[width][height];
    // ...
}

You also hardcoded the direction values. Put them into a enum so you can easily refer to them by name. Also define the tag value in your organism classes as well, so you can check against ant::tag instead of using the hard-coded value. The magic numbers of my samples above should be also replaced of course.

class ant : public organism
{
public:
    static constexpr char tag = 'o';
}

Maybe it would be even better if the constructor of the environment could take two arguments denoting its size. This way you could easily run different experiments, without changing the code. You will need a dynamic array structure then, like std::vector.

GenerateRandomStartingLocations

This method would make a lot more sense if it actually returned a location. Now it simply returns a random number, this is not what the name implies. You also use it for generating a random direction. Make the generation of a random number a free function in some utility namespace. Then this function can actually return a location. Since the width and height of the environment is known, we don't event need the parameters. Furthermore, since this method should generate a starting location, we can also check whether the generated location is occupied or not directly in this method. This way you would get rid of a lot of duplicate code in your CreateStartPopulation method.

coordinates environment::GenerateRandomStartingLocations()
{
    coordinates res;
    do
    {
        res.xCoordinate = Util::Random(0, width);
        res.yCoordinate = Util::Random(0, height);
    }
    while (environmentBoard[res.xCoordinate][res.yCoordinate] != nullptr);

    return res;
}

Simplifications

void organism::ToggleMoveAlternator()
{
    // make use of the not operator here, to get rid of the if 
    moveAlternator = !moveAlternator;
}

bool doodlebug::Death()
{
    // again get rid of the if and directyl return the expression
    return starveCounter == 3;
}

The use of ranged-based loops can make your loops a lot more readable:

void environment::AntsAct()
{
    for (auto const& row : environmentBoard)
    {
        for (auto& orga : row)
        {
            if (orga && orga->GetIdentifierTag() == ant::tag)
            {
                orga->ToggleMoveAlternator();
                orga->Move();
            }
        }
    }
    // ...
}

Better yet, the use of lambdas can even further increase readability.

void environment::LoopOrganisms(char tag, std::function<void(organism*)> func)
{
    for (auto const& row : environmentBoard)
    {
        for (auto& orga : row)
        {
            if (orga && orga->GetIdentifierTag() == tag)
                func(orga);
        }
    }
}

void environment::AntsAct()
{
    LoopOrganisms(ant::tag, [](auto && orga)
    {
        orga->ToggleMoveAlternator();
        orga->Move();
    });

    LoopOrganisms(ant::tag, [](auto && orga)
    {
        orga->IncrementBreedingCycle();
        if (orga->TimeToBreed())
            orga->Breed();
    });
}

Use = default

When your constructors/destructors don't do anything special from the default generated ones, explicitly write that:

virtual ~organism() = default;

Use override

When you have a base class with virtual methods and sub-classes that overwrite them, use the override keyword.

void Move() override;   
bool TimeToBreed() override;

This will also prevent bugs, where you meant to override a method, but have a typo in the name or parameter list and actually create a new method. You will get an error if there's no virtual base method.

Other things Don't use this to access your class members. It's not necessary in C++ and just clutters the code.

Consider using a better random number generation than srand, e.g. those provided in <random>.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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