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;
}
}
1 Answer 1
Here are some things that may help you improve your code.
Use the appropriate #include
s
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
.
-
\$\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– Herodegon2021年02月24日 19:00:06 +00:00Commented 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– Herodegon2021年02月24日 19:07:10 +00:00Commented 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– Edward2021年02月24日 20:49:03 +00:00Commented 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– Herodegon2021年02月24日 22:54:18 +00:00Commented 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-userand()
. It's a horribly over-engineered monstrosity if all you want is a sequence of arbitrary numbers. \$\endgroup\$Mark– Mark2021年02月24日 23:35:08 +00:00Commented Feb 24, 2021 at 23:35