Critique my program, give advice

Pages: 12
closed account 3R5fjE8b (226)
I'm creating a game-engine for a text-adventure RPG, this is my first real attempt at creating a large project so be gentle! Thank you for your time in advance. Layout of post is header file followed by it's cpp file. Any advice, criticism, compliments are very welcome, I am still learning!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
#include <iostream>
#ifndef EQUIPMENT_H
#define EQUIPMENT_H

class equipment
{
	public:
        std::string itemName;   
		equipment(std::string setName,int setdamage, int setarmor);
		std::string getName();
		int getDamage();
		int getArmor();
		void setDamage(int newDamage);
		void setArmor(int newArmor);
		
    private:
            int damage, armor;
		
};

#endif 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
 
#include <iostream>
#include "equipment.h" 

equipment::equipment(std::string setName,int setdamage, int setarmor)
{
                         itemName = setName;
                         damage = setdamage;
                         armor = setarmor;
}

std::string equipment::getName()
{
            return itemName;
}

int equipment::getDamage()
{
    return damage;
}

int equipment::getArmor()
{
    return armor;
}

void equipment::setDamage(int newDamage)
{
     damage = newDamage;    
}

void equipment::setArmor(int newArmor)
{
     armor = newArmor;
}


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
 
#include <iostream>
#include <vector>
#include "equipment.h"
#ifndef PLAYER_H
#define PLAYER_H


class player
{
	public:
           
           std::string username;
           std::vector<equipment> inventory;
           
           //EQUIPPED ITEM MANAGEMENT
           
           std::string weaponEquipped;
           void setArmorPoints (int newArmor);
           int getArmor();
           void setAttackPoints(int newAttack);
           int getAttack();
           void setHitpoints(int newHitpoints);
           int getHitpoints();
           
           //ITEM AND INVENTORY MANAGEMENT
           
           void displayInventory();
           void displayHUD();
           
           bool isWeaponSlotAvailable();
           
           void unequipSword();
           void equipSword();
           void addSword();
           void removeSword();
           
           void unequipAxe();
           void equipAxe();
           void addAxe();
           void removeAxe();

		player();
		
		private:
                int attackPoints, armorPoints, hitpoints;
                bool weaponSlot;
		


};

#endif


1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
 
#include <iostream>
#include <vector>
#include "equipment.h"
#include "player.h" 




player::player()
{
                setArmorPoints(0);
                setAttackPoints(1);
                setHitpoints(10);
                weaponSlot = false;
                
}

void player::setArmorPoints(int newArmor)
{
     armorPoints = newArmor;
}

int player::getArmor()
{
    return armorPoints;
}

void player::setAttackPoints(int newAttack)
{
     attackPoints = newAttack;
}

int player::getAttack()
{
    return attackPoints;
}

void player::setHitpoints(int newHitpoints)
{
     hitpoints = newHitpoints;
}

int player::getHitpoints()
{
    return hitpoints;
}
     
void player::displayInventory()
{
     if ( inventory.empty() )
     {
          std::cout << "You have nothing in your inventory." << std::endl;
     }
     else
     {
         std::cout << "You have " << inventory.size() << " items in your inventory." << std::endl;
         
         for ( int x = 0; x < inventory.size(); x++ )
         {
             std::cout << inventory[x].itemName;
             if ( inventory[x].itemName == weaponEquipped )
             {
                  std::cout << "   [Equipped]" << std::endl;
             }
             else
             {
                 std::cout << std::endl;
             }
         }     
     }
}

void player::displayHUD()
{
     std::cout << "Attack  " << getAttack() << " | Armor  " << getArmor() << " | Hitpoints  " << getHitpoints() << std::endl;
}

bool player::isWeaponSlotAvailable()
{
     if ( weaponSlot == true )
     {
          return false;
     }
     else
     {
         return true;
     }
}

void player::unequipSword()
{
     weaponSlot = false;
     weaponEquipped = "None";
     attackPoints -= 2;
     armorPoints -= 0;
     
}

void player::equipSword()
{
     for ( int x = 0; x < inventory.size(); x++ )
     {
         if ( inventory[x].getName() == "Sword" && isWeaponSlotAvailable() == true )
         { 
              setAttackPoints(getAttack() + inventory[x].getDamage());
              weaponSlot = true;
              weaponEquipped = "Sword";
         }
         else if ( isWeaponSlotAvailable() == false ) 
         { 
              std::cout << "You should unequip yourself first!" << std::endl; 
         }
         
     }
     if (weaponSlot == false) 
     {
                    std::cout << "You don't have a sword in your inventory." << std::endl;
     }
}
   
void player::addSword()
{    
     equipment sword("Sword", 2, 0);
     inventory.push_back(sword);
     
}

void player::removeSword()
{
     for ( int x = 0; x < inventory.size(); x++ )
     {
         if ( inventory[x].itemName == "Sword" )
         {
              inventory.erase(inventory.begin() + x);
         }
     }
}

void player::unequipAxe()
{
     weaponSlot = false;
     weaponEquipped = "None";
     attackPoints -= 4;
     armorPoints -= 0;
}


void player::equipAxe()
{
     for ( int x = 0; x < inventory.size(); x++ )
     {
         if ( inventory[x].getName() == "Axe" && isWeaponSlotAvailable() == true )
         { 
              setAttackPoints(getAttack() + inventory[x].getDamage());
              weaponSlot = true;
              weaponEquipped = "Axe";
         }
         else if ( isWeaponSlotAvailable() == false ) 
         { 
              std::cout << "You should unequip yourself first!" << std::endl; 
         }
         
     }
     if (weaponSlot == false) 
     {
                    std::cout << "You don't have a axe in your inventory." << std::endl;
     }
}

void player::addAxe()
{
     equipment axe("Axe", 4, 0);
     inventory.push_back(axe);
}

void player::removeAxe()
{
     for ( int x = 0; x < inventory.size(); x++ )
     {
         if ( inventory[x].itemName == "Axe" )
         {
              inventory.erase(inventory.begin() + x);
         }
     }
}

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
 
#include <iostream>
#include <fstream>
#ifndef MAP_H
#define MAP_H


class map
{
	public:
           //Map Defining Variables
           const static int height = 20;
           const static int width = 20;
           std::string game_map[height][width];
           
           //Map Navigation Variables
           bool gameOver;
           int xCoord, yCoord;
           std::string currentLocation, lastLocation;
           
           //Map Navigation Functions
           void updateCurrentLocation();
           void updateLastLocation();
           std::string getCurrent();
           std::string getLast();
           void moveNorth();
           void moveSouth();
           void moveEast();
           void moveWest();
           

		map();

};

#endif  

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
 
#include <iostream>
#include <fstream>
#include "map.h"


map::map()
{
          std::ifstream load_game_map("Dungeon/Dungeon-Map.txt");
          if ( load_game_map.fail() )
          {
               std::cout << "An error occured while loading the map." << std::endl;
          }
          
          else
          {
              for ( int x = 0; x < height; x++ )
              {
                  for ( int y = 0; y < width; y++ )
                  {
                      load_game_map >> game_map[x][y];
                  }
              }
              load_game_map.close();
          }
          xCoord = 1;
          yCoord = 1; 
          updateCurrentLocation();
          gameOver = false;
}

void map::updateCurrentLocation()
{
     currentLocation = game_map[xCoord][yCoord];
}

void map::updateLastLocation()
{
     lastLocation = currentLocation;
}

std::string map::getCurrent()
{
            return currentLocation;
}

std::string map::getLast()
{
            return lastLocation;
}

void map::moveNorth()
{
     updateLastLocation();
     xCoord--;
     updateCurrentLocation();
}

void map::moveSouth()
{
     updateLastLocation();
     xCoord++;
     updateCurrentLocation();
}

void map::moveEast()
{
     updateLastLocation();
     yCoord++;
     updateCurrentLocation();
}

void map::moveWest()
{
     updateLastLocation();
     yCoord--;
     updateCurrentLocation();
}
Last edited on
MiiNiPaa (8886)
I just skimmed first sinippets, but I noticed that you might have problems with program architecture.
1
2
 
void equipment::setDamage(int newDamage)
void equipment::setArmor(int newArmor)
Can Rusty Sword suddenly change and do 100 damage instead of 1? If not there should not be setters for these values. Ideally equipment should be immutable.

void player::setArmorPoints(int newArmor)You are manually adjusting player stats even when those should be calculated depending on equipment/level

1
2
3
4
5
 
void player::unequipSword()
void player::equipSword()
void player::addSword()
void player::removeSword()
//Same for axe
If you will need to remove Axe from the game and replace it with Spear, you will need to rewrite your player class. Ideally you should have functions like
1
2
3
4
 
void player::addEquipment(const equipment& stuff);
void player::addEquipment(InvertoryIndex i);
void player::equip(InvertoryIndex i);
void player::unequip(); //Unequips currently hold weapon

closed account 3R5fjE8b (226)
Good point! I changed that since I don't want any weapon changing after creation.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
 
#include <iostream>
#ifndef EQUIPMENT_H
#define EQUIPMENT_H

class equipment
{
	public:
           
		equipment(std::string setName,int setdamage, int setarmor);
		std::string getName();
		int getDamage();
		int getArmor();

		
    private:
            int damage, armor;
            std::string itemName;
		
};

#endif




Also, I'm not sure of how to go about writing the function definitions for such a broad function like these. Could you give an example?

1
2
3
4
5
 
void player::addEquipment(const equipment& stuff);
void player::addEquipment(InvertoryIndex i);
void player::equip(InvertoryIndex i);
void player::unequip(); //Unequips currently hold weapon
MiiNiPaa (8886)
For example:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
 
void player::addEquipment(const equipment& stuff)
{
    invertory.push_back(stuff);
}
//Usage
p1.addEquipment(EQUIPMENT::sword);

void player::addEquipment removeEquipment(InvertoryIndex i) //My mistake
{
//You should know where in your invertory needed equipment is.
    invertory.erase(invertory.begin() + i);
}
//Alternatively you can use search by name:
//void player::removeEquipment(const std::string& i);
//Or instead of vector use another container, like set or multiset

void player::equip(InvertoryIndex i) 
{
//Your variant
//    if (weaponSlot) {
//        std::cout << "You should unequip your current weapon first";
//        return;
//    }
//Automatic unequip:
    if (weaponSlot)
        unequip(); 
    setAttackPoints(getAttack() + inventory[i].getDamage());
    weaponSlot = true;
    weaponEquipped = inventory[i].getName();

closed account 3R5fjE8b (226)
Ohhh I see! But I have a question, when I equip an item, I need my armorPoints and attackPoints to reflect whatever item was equipped, how would i do that like this? It gets added when I equip but when I unequip, It won't subtract the attack
Last edited on
closed account 3R5fjE8b (226)
Also, I can't get the addEquipment function to work because creating a equipment object takes parameters
MiiNiPaa (8886)
1) Store not only the name, but Eqipment itself in weaponEquipped. When you unequip you just substract weaponEquipped.getDamage() etc before setting weaponEquipped to NONE.
2) Store base attack (of character) separately from weapon.
So your GetDamage() function will calculate damage dynamically like:
1
2
3
 
{
    return baseDamage + weaponEquipped.getDamage();
}
That way you do not need to substract anything manually.
closed account 3R5fjE8b (226)
Void addEquipment(equipment sword("Sword", 4, 0))
I can't get this to work

closed account 3R5fjE8b (226)
Sorry I'm trying to grasp
MiiNiPaa (8886)
You can do either:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
//1st
class EQUIPMENT //All equipmnet in your game
{
    static const equipment sword("Sword", 4, 0);
    //Etc
};
//Usage :
p1.addEquipment(EQUIPMENT::sword);

//Also you can use std::map to store equipment (preferable).

//2nd
p1.addEquipment(equipment("Sword", 4, 0)); 
p1.addEquipment({"Sword", 4, 0}); // C++11 style
Last edited on
AbstractionAnon (6954)
A couple of other comments:
1) Avoid making class members public. You have getters and setters for changing your variables. Accessing public class members tends to make code unwieldy.

2) I'd suggest avoiding the use of map as a class name and map.h as a header file. I realize you're not using std::map nor bringing in the full std namespace with using namespace std; so there is no conflict now, however as MiNiPaa suggested at some point you might want to use std::map to store equipment. Now things start to get confusing.

closed account 3R5fjE8b (226)
How does map work? Never seen it before. I also noticed I had variables as public, I changed that, I had it like that while testing.
MiiNiPaa (8886)
http://www.cplusplus.com/reference/map/map/
http://en.cppreference.com/w/cpp/container/map
http://en.cppreference.com/w/cpp/container/map/map

Example:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
 
enum class EQUIPMENT
{
    sword,
    axe,
};

const std::map<EQUIPMENT, equipment> loot = {
    { EQUIPMENT::sword, {"Sword", 2, 0} },
    { EQUIPMENT::axe, {"Axe", 4, 0} },
};
//or
const std::map<std:string, equipment> loot = {
    { "sword", {"Sword", 2, 0} },
    { "axe", {"Axe", 4, 0} },
};

//Usage:
p1.addEquipment(loot[EQUIPMENT::sword]);
//or
p1.addEquipment(loot["sword"]);
closed account 3R5fjE8b (226)
What do I use enum class for and how would I incorporate that? As of right now that code snippet gives me an error?

Not sure what file I would code that either
closed account 3R5fjE8b (226)
Gives me an error, says forbids declaration with no type

1
2
3
4
 
enum class equipment {
     sword,
     axe,
};
MiiNiPaa (8886)
You already have class equipment. So you need to name enum something else: all caps will do or you can use other name.

You might use thah enum as enumeration of all possible equipment.
closed account 3R5fjE8b (226)
EQUIPMENT and ITEMS diesnt work either, same error
MiiNiPaa (8886)
Are your copiler supprot C++11? Do you have C++11 support enabled?
closed account 3R5fjE8b (226)
I have no idea, I have dev c++
MiiNiPaa (8886)
Bloodshed or Orwell?
Pages: 12