5
\$\begingroup\$

Here's an RPG I'm working on that uses only standard C++ libraries. I'm having trouble conceptualizing a coordinate system for characters spawned in a map, and the inventory/item system is only a shell at the moment. However, I've created a small demo including some of the features that will be present in the full product (Game::Test()). This is my first real attempt at a game, so I would love to hear what you think of what I have so far, and ways I could improve.

*Note: I didn't include my inventory system, since there's barely any code for it, and what is there is all concept anyways.

main.cpp

#include <vector>
#include <cstring>
#include <cstdlib>
#include <ctime>

#include "Game.h"

int main() {
    //Seed to use for run;
    //!FIX; OVERHAUL TO HAVE SEED GENERATED BY RUN INSTEAD OF INSTANCE
    srand(time(NULL));
    
    Game myRpg;
    char charInput;
    std::string strInput;
    
    //Run Test Mode (Has Access To Basic Game Mechanics)
    std::cout << "Run test? Enter \'Y\' for test." << std::endl;
    std::cin >> charInput;
    
    //Test is all that exists right now. Always enter Y to run game
    if (charInput == 'Y') {
        myRpg.Test();
    }
            
    return 0;
}

Game.h

#ifndef GAME_H
#define GAME_H

#include <iostream>
#include <cstring>
#include <vector>

#include "Characters.h"
#include "Maps.h"
#include "Player.h"

class Game {
    public:
        Game() {Clear(); Intro(); myChar.CharacterCreation();};
        void Clear();
        
        void Intro();
        
        void Test();
        void Run();
    
    private:
        //!FIX; `OUT-OF-RANGE` CRASH WHEN INITIALIZING MAP LIST
        /*===WIP===
        Maps mapLib;
        */
    
        Characters charLib; //!Will use `(mapLib.size())` once Maps fixed
        Characters* myChars = &charLib;
        
        Player myChar;
        Character* player = myChar.p;
        
        //Counts num turns player has made since init; Currently unused
        int time;
};

#endif

Game.cpp

#include <iostream>
#include <fstream>
#include <cstring>
#include <cstdio>
#include <cstdlib>
#include <ctime>
#include <vector>

#include "Game.h"

void Game::Clear() {
    for(unsigned int i = 0; i < 50; i++) {
        std::cout << std::endl;
    }
}

//Displays game intro using text file "intro.txt"
void Game::Intro() {
    //Opens text file "intro.txt"
    std::ifstream intro;
    intro.open("intro.txt");
    
    //Outputs text from file
    if(intro.is_open()) {
        std::string str;
        //While file has not reached the end
        while(!(intro.eof())) {
            getline(intro, str);
            std::cout << str << std::endl;
        }
        std::cout << std::endl << std::endl;
        intro.close();
    }
}

//Mode to run basic battle functions
void Game::Test() {
    //Clear previous screen
    Clear();
    
    /*!===TEST; Test() Start===
    std::cout << "Test Start" << std::endl;
    */
    
    //Create character and single opponent
    Character opponent;
    opponent = charLib.CreateChar("Bandit");
    Character* opp = &opponent;
    
    //Create Default Map
    Map default_map; //!FIX; Find Seg. Fault (Map.cpp)
    default_map.Print();
    
    std::cout << "As you sail across the ocean, you come across a bandit," << std::endl
              << "floating over the water. You pull him aboard, when      " << std::endl
              << "suddenly he jolts up, knife readied in hand. It was a  " << std::endl
              << "trick!" << std::endl;
    std::cout << "Press \'enter\' to continue.";
    std::cin.get();
    std::cin.ignore();
    std::cout << std::endl;
    
    player->Battle(opp);
    
    if (player->IsAlive() == 0) {
        std::cout << "The treasure remains lost. GAME OVER" << std::endl;
        return;
    }
    
    std::cout << "You won!- but the treasure is still out there somewhere..." << std::endl;
    
    /*!===TEST; Test() End===
    std::cout << "Test Successful!" << std::endl;
    */
    
}

Player.h

#ifndef PLAYER_H
#define PLAYER_H

#include <iostream>
#include <cstring>
#include <vector>

#include "Character.h"  

class Player {
    public:
        Player() {p = &player; lv = 1; player.SetSym('@');}
        int GetLv();
        
        void PlayerTurn();
        void Battle(Character *opponent);          //Option 1
        void Defend();          //Option 2
        void OpenInventory();   //Option 3
        void RunAway();         //Option 4
        
        void CharacterCreation();
        
        Character* p;
    
    private:
        Character player;
        int lv;
};

/*===WIP===//
struct Ability {
    std::string name;
    int lv;
    
    int cost(int lv) {
        return (lv);
    }
    
    bool canAfford(int lv, int mp) {
        return cost(lv) <= mp;
    }
        
    void show(int lv) {
        std::cout << "[" << cost() << "] mp\n";
    }
};

class Job {
    public:
        void PrintAbilities(int lv);
        
    private:
        std::string name;
        std::vector<Ability> abilities;
        int numAbilities() {return abilities.size();}
        
        Ability const &operator[](int index) const {
            return abilities.at(index);
        }
    
};
//=========*/

#endif

Player.cpp

#include <iostream>
#include <cstring>
#include <cstdio>
#include <vector>
#include <iomanip>

#include "Player.h"

//===WIP===
void Player::PlayerTurn() {
    char usrInput = ' ';
    
    std::cout << "Your turn. Press \'?\' for help." << std::endl;
    usrInput = getchar();
    
    switch(usrInput) {
        case 'w' || 'a' || 's' || 'd':
            break;
        case '?':
            break;
        case 'i':  //!FIX; FINISH INVENTORY/ITEM SYSTEM
            std::cout << "Not functional at this time." << std::endl;
            break;
        case '*':
            break;
    }
    
    if (player.IsAlive() == 0) {
        std::cout << "Your adventure has come to an end." << std::endl;
    }
}

void Player::Battle(Character *opponent) {
    player.Battle(opponent);
}   

void Player::CharacterCreation() {
    std::string usrStr;
    
    //Set Player Name
    std::cout << "What's your name? ";
    getline(std::cin, usrStr);
    player.SetName(usrStr); //Set name as usrStr input
    std::cout << std::endl;
    
    //Set Player Stats
    bool isYes;
    bool isRollOrSet;
    
    do {
        std::cout << "Would you like to \'roll\' or \'set\' your stats? "
                  << "Please enter \'roll\' or \'set\'" << std::endl;
        std::cin >> usrStr;
        if ((usrStr != "roll") && (usrStr != "set")) {
            std::cout << "Please enter an appropriate response." << std::endl;
        }
        else {
            do {
                bool setOrRoll = (usrStr == "roll");
                switch (setOrRoll) {
                    case 1: //Rolls stats
                        player.RollStats();
                        break;
                        
                    case 0: //Use a stat pool
                        player.SetStats();
                        break;
                }
                //Confirm with user that they want these stats; Display Stats
                std::cout << "Are these stats okay? Enter \'YES\' if so, or enter "
                          << "\'roll\' / \'set\' to redo your stats accordingly.\n";
                std::cout << std::setw(2);
                std::cout << "HP: " << player.GetHp() << std::endl;
                std::cout << "MP: " << player.GetMp() << std::endl;
                std::cout << "STR: " << player.GetStr() << std::endl;
                std::cout << "DEF: " << player.GetDef() << std::endl;
                std::cout << "MAG: " << player.GetMag() << std::endl;
                std::cout << "SPD: " << player.GetSpd() << std::endl;
                
                do {
                    std::cin >> usrStr;
                    
                    isYes = (usrStr == "YES");
                    isRollOrSet = ((usrStr == "roll") || (usrStr == "set"));
                    
                    //If they entered "YES"
                    if (usrStr == "YES") {
                        break;
                    }
                    //If they entered something other than "YES", "roll", or "set"
                    else if ((isYes == false) && (isRollOrSet == false)) {
                        std::cout << "Please enter an appropriate answer." << std::endl << std::endl;
                    }
                //Loops input
                } while((isYes == false) && (isRollOrSet == false));
            //Loops stat roll
            } while(isRollOrSet == true);
        }
    //Loops initial question
    } while(isYes == false);
}

/*===WIP===//
Job::PrintAbilities(int lv) {
    for(unsigned int i = 0; i < numAbilities; i++) {
        std::cout << "[" << i << "]";
        abilites[i].show(lv);
    }
}
//=========*/

Characters.h

#ifndef CHARACTERS_H
#define CHARACTERS_H

#include <iostream>
#include <vector>

#include "Character.h"

class Coord {
    public:
        Coord() {};
        void SetData(int x, int y) {
            this->x = x;
            this->y = y;
        }
        
        void XShift(int a) {x += a;}
        void YShift (int b) {y += b;}
        
        int GetX() {return x;}
        int GetY() {return y;}
        
    private:
        int x;
        int y;
};

/********************************************************************/

class Characters {
    public:
        Characters() {SetArchList();}
        Characters(int numMaps) {SetArchList(); charList.resize(numMaps);}
        void AddCharArch(Character _char, std::vector<Character> _chars);
        Character CreateChar(std::string archName);
        
    private:
        //Stores character archetypes
        std::vector<Character> archList;
        std::vector<std::vector<Character>> charList;
        /*!FIX; DYNAMICALLY ADD CHARACTERS FROM CURR MAP TO LIST
         * Reference == Map.cpp, `void Map::SpawnEntities() {};`  */
        
        void SetArchList();
};

#endif

Characters.cpp

#include <iostream>
#include <cstring>
#include <vector>

#include "Characters.h"

//Adds character archetypes to characterList
void Characters::AddCharArch(Character _char, std::vector<Character> _chars = {{"null"}}) {
    if (_chars.at(0).GetName() == "null") {
        //Copies character data _char
        archList.push_back(_char);
    }
    else {
        //Copies data from character vector _chars
        for(unsigned int i = 0; i < _chars.size(); i++) {
            archList.push_back(_chars.at(i));
        }
    }
}

//Retrieves character archetype data from characterList
Character Characters::CreateChar(std::string archName) {
    Character archType;
    for(unsigned int i = 0; i < archList.size(); i++) {
        Character* charListArch = &archList.at(i); //Creates a pointer for archetype in charList at i
        if(charListArch->GetName() == archName) {
            archType = charListArch->GetName();
        }
        else {
            continue;
        }
    }
    
    return archType;
}

void Characters::SetArchList() {
    //Archetype Info |Name     | hp| mp|str|def|mag|spd|sym|
    archList =      {{"Bandit" ,  15,  0,  5,  2,  0,  2,'b'},
                     {"Crew"   ,  24,  4,  8,  5,  2,  4,'c'},
                     {"Captain",  32,  8, 10,  8,  5,  6,'C'}};
};

Character.h

#ifndef CHARACTER_H
#define CHARACTER_H

#include <iostream>
#include <cstring>
#include <vector>

#include "Inventory.h"

class Character {
    public:
        Character() : name("Default"), hp(6), mp(6), str(6), def(6), mag(6), spd(6) {};
        Character(std::string name) : hp(12), mp(6), str(6), def(6), mag(6), spd(6) {this->name = name;}
        Character(std::string name, int hp, int mp, int str, int def, int mag, int spd, char symbol) {
            this->name = name;
            this->hp = hp * 3;
            this->mp = mp;
            this->str = str;
            this->def = def;
            this->mag = mag;
            this->spd = spd;
            this->symbol = symbol;
        }
        
        void SetName(std::string);                //Create character name
        std::string GetName() {return name;}      //Get character's name
        
        void SetStats();      //Set stat values using point pool
        void RollStats();     //Roll d12 for character's start stats
        
        void SetHp(int hp) {this->hp = hp;}
        void SetMp(int mp) {this->mp = mp;}
        void SetStr(int str) {this->str = str;}
        void SetDef(int def) {this->def = def;}
        void SetMag(int mag) {this->mag = mag;}
        void SetSpd(int spd) {this->spd = spd;}
        void SetSym(char symbol) {this->symbol = symbol;}
        
        int GetHp() const {return hp;}
        int GetMp() const {return mp;}
        int GetStr() const {return str;}
        int GetDef() const {return def;}
        int GetMag() const {return mag;}
        int GetSpd() const {return spd;}
        char GetSym() const {return symbol;}
        
        void Battle(Character *opponent);
        
        bool IsAlive() const {return hp > 0;}        //Check if character's alive
        void TakeDamage(Character *opponent);        //Take damage
        int GetDamage(Character* opponent) const;
        
    private:
        std::string name;  //Character name
        int hp;            //Health of character
        int mp;            //Magic points of character
        int str;           //Strength of character
        int def;           //Defense of character
        int mag;           //Magic skill of character
        int spd;           //Speed of character
        
        char symbol;
};

#endif

Character.cpp

#include <iostream>
#include <cstring>
#include <cstdlib>

#include "Character.h"

void Character::SetName(std::string name) {
    this->name = name;
}

//Allows the user to freely set their character's stats from a pool of 36
void Character::SetStats() {
    int pointPool = 36;
    
    for(unsigned int i = 0; i < 6; i++) {
        int usrInt = 0;
        bool goodStat = false;
        
        do {
            std::cout << "You have " << pointPool << " points remaining.\n";
            std::cout << "How many points for ";
            switch(i) {
                case 0:
                    std::cout << "HP? ";
                    std::cin >> usrInt;
                    break;
                    
                case 1:
                    std::cout << "MP? ";
                    std::cin >> usrInt;
                    break;
                    
                case 2:
                    std::cout << "STR? ";
                    std::cin >> usrInt;
                    break;
                    
                case 3:
                    std::cout << "DEF? ";
                    std::cin >> usrInt;
                    break;
                    
                case 4:
                    std::cout << "MAG? ";
                    std::cin >> usrInt;
                    break;
                    
                case 5:
                    std::cout << "SPD? ";
                    std::cin >> usrInt;
                    break;
            }
            // Check if player choice was appropriate
            if (usrInt > pointPool) {
                std::cout << "Not enough points." << std::endl;
            }
            else if (usrInt < 0) {
                std::cout << "Can't allocate less than 0 points." << std::endl;
            }
            //Set Stats
            else {
                switch(i) {
                    case 0:
                        SetHp(usrInt * 3);
                        break;
                        
                    case 1:
                        SetMp(usrInt);
                        break;
                        
                    case 2:
                        SetStr(usrInt);
                        break;
                        
                    case 3:
                        SetDef(usrInt);
                        break;
                        
                    case 4:
                        SetMag(usrInt);
                        break;
                        
                    case 5:
                        SetSpd(usrInt);
                        break;
                }
                goodStat = true;
            }       
        } while(goodStat == false);
        
        //Subtract player choice from point pool
        pointPool -= usrInt;
        
        /*!===TEST; SetStats() Interval===
        std::cout << std::endl;
        std::cout << "Test: Interval " << i << std::endl;
        */
    }
}

void Character::RollStats() {
    int pointPool = 36;
    
    for(unsigned int i = 0; i < 6; i++) {
        int randNum;
        
        //Get random number 1-8 for stat allocation
        do {
            randNum = (rand() % 8) + 1;
            if(randNum <= pointPool) {
                break;
            }
        } while(randNum > pointPool);
        
        //Set stat using randNum
        switch(i) {
            case 0:
                if (randNum <= 3) {
                    randNum = 4;
                }
                SetHp(randNum * 3); //Set Health
                break;
            case 1:
                SetMp(randNum); //Set Magic Points
                break;
            case 2:
                SetStr(randNum); //Set Strength
                break;
            case 3:
                SetDef(randNum); //Set Defense
                break;
            case 4:
                SetMag(randNum); //Set Magic Skill
                break;
            case 5:
                SetSpd(randNum); //Set Speed
                break;
        }
        pointPool -= randNum; //Remove randNum from pointPool
    }
}

void Character::Battle(Character *opponent) {
    bool att; //Attacker
    bool opp; //Opponent
    
    do {
        bool whoseFaster = GetSpd() > opponent->GetSpd();
        
        switch(whoseFaster) {
            case 1: //Player's faster
                opponent->TakeDamage(this);
                std::cout << name << " hits " << opponent->GetName()
                          << " for " << opponent->GetDamage(this) << " points." << std::endl;
                
                //If the opponent died from attack, end battle
                if (IsAlive() == false) {
                    break;
                }
                
                TakeDamage(opponent);
                std::cout << opponent->GetName() << " hits " << name
                          << " for " << GetDamage(opponent) << " points." << std::endl;
                break;
            case 0: //Opponent's faster
                TakeDamage(opponent);
                std::cout << opponent->GetName() << " hits " << name
                          << " for " << GetDamage(opponent) << " points." << std::endl;
                          
                //If player died from attack, end battle
                if (IsAlive() == false) {
                    break;
                }
                
                //Else, player attacks
                opponent->TakeDamage(this);
                std::cout << name << " hits " << opponent->GetName()
                          << " for " << opponent->GetDamage(this) << " points." << std::endl;
                break;
        }
        
        std::cout << GetName() << " has " << GetHp() << " HP remaining." << std::endl
                  << opponent->GetName() << " has " << opponent->GetHp() << " hp remaining." << std::endl
                  << std::endl;
        
        att = IsAlive();
        opp = opponent->IsAlive();
    } while( (att == true) && (opp == true) );
    
    switch(att) {
        case 1:
            std::cout << opponent->GetName() << " was slain." << std::endl;
            break;
        case 0:
            std::cout << name << " was cut down." << std::endl;
            break;
    }
}

//Character takes damage from parameter
//  |Recipient|            |Attacker|
void Character::TakeDamage(Character *opponent) {
    int dmg = GetDamage(opponent);
    
    hp -= dmg;
}

//Returns damage value
//  |Recipient|          |Attacker|
int Character::GetDamage(Character* opponent) const {
    int dmg = (opponent->GetStr() - def);
    if (dmg < 1) {
        dmg = 1;
    }
    
    return dmg;
}

Maps.h

#ifndef MAPS_H
#define MAPS_H

#include <iostream>
#include <cstring>
#include <vector>

#include "Map.h"

class Maps {
    public:
        Maps() {SetMapTypes(); SetMapNumTypes(); GenMaps(1);}
        Maps(int numMaps) {SetMapTypes(); SetMapNumTypes(); GenMaps(numMaps);}
        void SetCurrMap();
        void PrintCurrMap() const;
        
    private:
        //Map types and num of maps per type
        std::vector<std::string> mapTypes;
        std::vector<int> mapNumTypes;
        
        //Collection of Map
        std::vector<Map> maps;
        
        //Copy of map for gameplay 
        Map currMap;
        
        void SetMapTypes();
        void SetMapNumTypes();
        void GenMaps(int numMaps);
};

#endif

Maps.cpp

#include <iostream>
#include <cstring>
#include <vector>
#include <cstdlib>

#include "Maps.h"
#include "Map.h"

void Maps::SetMapTypes() {
    mapTypes = {"test_area", "ship_battle"};
}

void Maps::SetMapNumTypes() {
    //             test_area, ship_battles
    mapNumTypes = {        1,            1};
}

void Maps::GenMaps(int numMaps) {
    srand(time(NULL));
        
    for(int k = 0; k < numMaps; k++) {  
        //Set random map type from library
        int randType = rand() % mapTypes.size();
        std::string mapType = mapTypes.at(randType);
            
        //Get num of maps for type; Set random map variation
        int mapVars = mapNumTypes.at(randType);
        int randMapVar = rand() % mapVars;
            
        //Set map name
        std::string mapName = mapType;
        mapName.push_back(randMapVar);
        mapName.append(".txt");
            
        //Generate and store map copy
        Map map(mapName);
        maps.push_back(map);
    }
}

Map.h

#ifndef MAP_H
#define MAP_H

#include <iostream>
#include <cstring>
#include <vector>

#include "Characters.h"

class Map {
    public:
        Map() {this->name = "default"; GenMap(); SpawnEntities();}
        Map(std::string name) {this->name = name; GenMap(); SpawnEntities();}
        void Print() const;
    
    private:
        std::string name;
        std::vector<std::vector<char>> map;

        void GenMap();
        void SpawnEntities();
};

#endif

Map.cpp

#include <iostream>
#include <fstream>
#include <cstring>
#include <cstdlib>
#include <ctime>
#include <vector>

#include "Map.h"

void Map::GenMap() {
    /*!===TEST; GenMap() Start===
    std::cout << "Map Gen Start" << std::endl;
    */
    
    int width = 0;
    int height = 0;
    
    //If name is "default", generate pre-made test map
    if (name == "default") {
        map = {{'~','~','~','~','~','~','~','~','~'},
               {'~','~','~','~','~','~','~','~','~'},
               {'~','~','~','~','-','~','~','~','~'},
               {'~','~','~','|','*','|','~','~','~'},
               {'~','~','~','|','b','|','~','~','~'},
               {'~','~','~','|','@','|','~','~','~'},
               {'~','~','~','~','-','~','~','~','~'},
               {'~','~','~','~','~','~','~','~','~'},
               {'~','~','~','~','~','~','~','~','~'}};
    }
    else {
        //Open map file of the same name as parameter
        std::ifstream mapFile;
        mapFile.open(name);
        
        //Copy contents of map to 2D vector
        if (mapFile.is_open()) {
            std::string str;
            getline(mapFile, str);
            
            
        } //Closes if statement
        mapFile.close();
        
    } //Closes else statement
    /*!===TEST; GenMap() END===
    std::cout << "Map Gen Successful!" << std::endl;
    */
}

//===WIP===
void Map::SpawnEntities() {
    //Do not spawn entities when map is default
    if (name == "default") {
        return;
    }
    
    int randNum1 = rand() % 16;
    
    //Spawn Player
    for(size_t i = 0; i < map.size(); i++) { //Row size
        bool placedPlayer = false;
        for(size_t k = 0; k < map.at(i).size(); k++) { //Column size
            int thisRandNum = rand() % 16;
            if((thisRandNum == randNum1) && (map[i][k] == '*')) {
                map[i][k] = '@';
                placedPlayer = true;
                break;
            }
        }
        if(placedPlayer == true) {
            break;
        }
    }
    
    //Spawn enemies
    int mapSeverity = (rand() % 5) + 1; //Enemy # multiplier between 1-5
    int numEntities = ((rand() % 4) + 1) * mapSeverity; //# of enemies spawned
    
    std::vector<char> includeEntities;
    std::vector<int> numSpawns;
    
    //!===TEST; mapSeverity Setting===
    if(name == "default") {
        mapSeverity = 1;
        numEntities = 3;
    }
    
    switch(mapSeverity) {
        case 5:
        case 4:
            includeEntities.push_back('C');
        case 3:
            includeEntities.push_back('c');
        case 2:
        case 1:
            includeEntities.push_back('b');
    }
    
    int randNum2 = rand() % 16;
    
    for(size_t i = 0; i < map.size(); i++) { //Row
        for(size_t k = 0; k < map.at(i).size(); k++) { //Column
            int thisRandNum = rand() % 16;
            if((thisRandNum == randNum2) && (map[i][k] == '*')) {
                
                    /*!===Finish Coding Entitiy Spawner===
                    //Needs: - Spawn number of each entity
                    //         using int numEntities
                    */
                    
            }
        }
    }
}

void Map::Print() const {
    for(size_t i = 0; i < map.size(); i++) {
        for(size_t k = 0; k < map.at(i).size(); k++) {
            std::cout << map[i][k];
        }
        std::cout << std::endl;
    }
}               
\$\endgroup\$

1 Answer 1

10
\$\begingroup\$

Here are some things that may help you improve your code.

Use the appropriate #includes

The code uses std::string but uses #include <cstring>. That's an error, because std::string is in <string> and not in the legacy C <cstring> header. Similarly, main does not need and should not #include <vector> because nothing from that header is used there.

Label intentional fallthrough

The code includes this switch:

switch(mapSeverity) {
    case 5:
    case 4:
            includeEntities.push_back('C');
    case 3:
            includeEntities.push_back('c');
    case 2:
    case 1:
            includeEntities.push_back('b');
}

This has a number of fallthroughs, which are when execution continues from previous cases. It's not clear whether these are intentional or not. If they are not, it's an error and you need to insert break; statements. If they are, and if your compiler is C++17 compliant, you should use the [[fallthrough]] attribute. At the very least, a comment would be useful.

Consider using a better random number generator

The code contains a number of attempts at random number generation that look like this:

int mapSeverity = (rand() % 5) + 1; //Enemy # multiplier between 1-5
int numEntities = ((rand() % 4) + 1) * mapSeverity; //# of enemies spawned

If you are using a compiler that supports at least C++11, consider using a better random number generator. In particular, instead of rand, you might want to look at std::uniform_int_distribution and friends in the <random> header.

Don't use std::endl if you don't really need it

The difference betweeen std::endl and '\n' is that '\n' just emits a newline character, while std::endl actually flushes the stream. This can be time-consuming in a program with a lot of I/O and is rarely actually needed. It's best to only use std::endl when you have some good reason to flush the stream and it's not very often needed for simple programs such as this one. Avoiding the habit of using std::endl when '\n' will do will pay dividends in the future as you write more complex programs with more I/O and where performance needs to be maximized.

Don't loop on eof()

The code currently contains this in Game.cpp:

void Game::Intro() {
        //Opens text file "intro.txt"
        std::ifstream intro;
        intro.open("intro.txt");
        
        //Outputs text from file
        if(intro.is_open()) {
                std::string str;
                //While file has not reached the end
                while(!(intro.eof())) {
                        getline(intro, str);
                        std::cout << str << std::endl;
                }
                std::cout << std::endl << std::endl;
                intro.close();
        }
}

It's almost always incorrect to loop on eof() while reading a file. The reason is that the eof indication is only set when an attempt is made to read something from the file when we're already at the end. See this question for more details on why using eof is usually wrong.

Here's how I'd rewrite that function:

void Game::Intro() {
    std::ifstream intro{"intro.txt"};
    std::string str;
    while (std::getline(intro, str)) {
        std::cout << str << '\n';
    }
    std::cout << "\n\n";
}

Better still:

void Game::Intro() {
    std::ifstream intro{"intro.txt"};
    std::cout << intro.rdbuf() << "\n\n";
}

Don't write getters and setters for every class

C++ isn't Java and writing getter and setter functions for every C++ class is not good style. Instead, move setter functionality into constructors and think very carefully about whether a getter is needed at all. In this code, the Character class is littered with code like this:

void SetHp(int hp) {this->hp = hp;}
int GetHp() const {return hp;}

If anything can modify the value, then it is not an invariant and so there is no advantage to having these functions. Simply declare such data members public instead. See C.131 for details.

Prefer in-class member initializers to constructors

The Character class has these three constructors:

Character() : name("Default"), hp(6), mp(6), str(6), def(6), mag(6), spd(6) {};
Character(std::string name) : hp(12), mp(6), str(6), def(6), mag(6), spd(6) {this->name = name;}
Character(std::string name, int hp, int mp, int str, int def, int mag, int spd, char symbol) { 
    this->name = name;
    this->hp = hp * 3;
    this->mp = mp;
    // etc. 
}

There's a lot that could be improved here. In particular, simply setting the values for each data member in the declaration makes this much more readable. See C.45 for details. As an aside, why would you triple only the received value for hp? And why would you have two different default values for hp?

Avoid confusing file names

There are a few files that differ only by one letter. There is Character.h and Characters.h and a similar situtation for Map.h and Maps.h. It would be a bit kinder to your fellow programmers (and yourself!) if you rename those to make confusion less likely. I'd suggest CharacterCollection for example.

Don't return pointers to private data

The Player class currently contains this:

class Player {
public:
    Player() {p = &player; lv = 1; player.SetSym('@');}
    // other stuff
    Character* p;
private:
    Character player;
    int lv;
};

The effect is that we have a public pointer p to private data player! This is bad because it means that any code can reach in and mess with the supposedly private data within the class, a violation of the information hiding principle. See C.9 for details.

The underlying problem appears to be some confusion about class structure. If a Player is a Character, which appears to be the intent here, then the better way to do this would be to have Character be a base class for Player. After I did a partial refactoring of your code, here's what it looks like:

// the Player class is derived from the Character class
class Player : public Character {
public:
    Player() : Character() { symbol ='@'; }
    int GetLv() const;
    void PlayerTurn();
    void Defend();
    void OpenInventory();
    void RunAway();
    void CharacterCreation();
private:
    int lv = 1;
};

There are a couple things to note here. First, I left the GetLv() function but made it const. Second, I removed the Battle function because the base class Character already implements what we need. Third, because I removed all of the trivial getters and setters from the Character class, the symbol is set directly within the constructor.

Simplify your code

The Battle() member function of the Character class is much more complex than it needs to be, especially if you take the previous bit of advice and have Player be a derived class of Character. Here's the rewritten Battle() function:

void Character::Battle(Character *opponent) {
    Character *attacker = this;
    Character *defender = opponent;
    // For the first round, the faster Character is attacker
    // Thereafter, they alternate
    if (attacker->spd < opponent->spd) {
        std::swap(attacker, defender);
    }
    while (defender->IsAlive()) {
        int damage = defender->TakeDamage(attacker);
        std::cout << attacker->name << " hits " << defender->name
                          << " for " << damage << " points.\n";
        std::swap(attacker, defender);
        // always report the player status
        std::cout << name << " has " << hp << " HP remaining.\n"
                  << opponent->name << " has " << opponent->hp 
                  << " hp remaining.\n\n";
    }
    // somebody is dead -- who was it?
    if (IsAlive()) {
        std::cout << opponent->name << " was slain.\n";
    } else {
        std::cout << name << " was cut down.\n";
    }
}

Understand overloading

The code currently contains this member function:

void Characters::AddCharArch(Character _char, std::vector<Character> _chars = {{"null"}}) {
    if (_chars.at(0).GetName() == "null") {
        //Copies character data _char
        archList.push_back(_char);
    }
    else {
        //Copies data from character vector _chars
        for(unsigned int i = 0; i < _chars.size(); i++) {
            archList.push_back(_chars.at(i));
        }
    }
}

There's a lot going on here that really cries out for refactoring! First, it looks like you're intending to allow either a single character or a collection of them to be appended to the internal archList. If that's the case, make two functions. Here's how I'd do that:

void Characters::AddCharArch(Character ch) {
    archList.emplace_back(ch);
}

void Characters::AddCharArch(const std::vector<Character>& chars) {
    archList.insert(archList.end(), chars.begin(), chars.end());
}

Note also that we use emplace_back for the first version since ch is already passed by value and that we use insert to append the list of passed characters to the end of archList.

\$\endgroup\$
7
  • \$\begingroup\$ Thank you for your advice! I'll definitely take this advice to heart and revise some of the things you mentioned in my program (the one about AddCharArch() is especially intriguing to me, and could help with how I've been managing class vectors). However, two notes: 1) The bleed-through in my case list was intentional, but considering you're the second person to mention it, I should have a comment mentioning it at the very least. \$\endgroup\$
    – Herodegon
    Commented Feb 24, 2021 at 19:00
  • \$\begingroup\$ 2) Part of the reason I made this was for a project where we a) could not use public member variables and b) had to initialize members through constructors. However, I think your point about my readability is valid despite these factors, and I'll take that into consideration when revising my code. 3) I don't wholly understand the point you made about having a direct pointer to a private member, or what a "base class" is. Pointers are something I struggle with, but even so if you could explain your point a bit more, I would greatly appreciate it! \$\endgroup\$
    – Herodegon
    Commented Feb 24, 2021 at 19:07
  • \$\begingroup\$ I've added more information to the answer to attempt to address your questions. As for your project requiring that members be initialized through constructors, these still are; it just takes a form you might not yet be used to seeing. Generally, it is good practice to have private member variables, but trivial getters and setters as you had, defeats the purpose and so you're better off without them in this case, and likely with your Coord class as well. \$\endgroup\$
    – Edward
    Commented Feb 24, 2021 at 20:49
  • \$\begingroup\$ I see what you mean. When it comes to the Coord class, that's an unfinished class which I was planning to use to store the coordinates of characters on a 2d vector, but I haven't figured out how I want to do it yet. \$\endgroup\$
    – Herodegon
    Commented Feb 24, 2021 at 22:54
  • \$\begingroup\$ In regards to your comments about <random>, I've looked at it, I've stared at it, I've rolled my eyes at it, and I've gone back to the easy-to-use rand(). It's a horribly over-engineered monstrosity if all you want is a sequence of arbitrary numbers. \$\endgroup\$
    – Mark
    Commented Feb 24, 2021 at 23:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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