Text Based RPG help

Can someone please give me some tips on how to clean this up and make it more object-oriented?

Initially I tried to create an enter function within the store(i.e. store.enter(mainPlayer)) and pass a pointer to player into the function but that didn't work out as I am still struggling to grasp the concept of pointers.

Also random encounter during rest is pretty messy. I would have liked to create the the random encounter within the rest function but failed as I'm sure using pointers is the solution.

I'm not asking for someone to just hand me over a solution I would just like some tips on improvement and hopefully help grasping pointers and their mechanics. Thank you in advance.

I'm a newbie or I would try to help you.

However, I've found using pastebin.com will make it easier for others to answer your question. Also, paste code here using the code tag. That might get you some answers!


EDIT: I'm playing your game now, by the way!

Can someone please give me some tips on how to clean this up and make it more object-oriented?

Initially I tried to create an enter function within the store(i.e. store.enter(mainPlayer)) and pass a pointer to player into the function but that didn't work out as I am still struggling to grasp the concept of pointers.

Also random encounter during rest is pretty messy. I would have liked to create the the random encounter within the rest function but failed as I'm sure using pointers is the solution.

I'm not asking for someone to just hand me over a solution I would just like some tips on improvement and hopefully help grasping pointers and their mechanics. Thank you in advance.

As a general rule of thumb, if you are passing a naked non-const pointer, that should be a "code smell" of sorts. That said, you can learn that lesson much later.


Pointers are actually relatively straight forward, at least compared to other aspects of C++!

Think of pointers if you will like GPS coordinates. They simply refer to a location in memory, however, like with GPS coordinates, when you resolve them, they are actually a physical location. For example, [color="#3366BB"][font="sans-serif"]

42.9837°N 81.2497°W

[/font] are the GPS coordinates of my hometown, but once you resolved them you have the city of London, Ontario.

With code pointers it much the same thing:

int i = 42;
int * intPointer = &i

Just like the GPS example, intPointer represents the location of the integer i. However, just like with coordinates, once you actually resolve them, or dereference in C++ parlance, you get the actual object itself, so:

cout << *intPointer;

Would print out 42, because that is the value at the location pointed to by i. At this point I suppose it makes sense to point out that '*' when applied to a pointer is what causes it to de-reference ( look at the value pointed at, not the location ), while the earlier used '&' when applied to a type says "give me the address/location of this thingy". The pointer is declared as the type it is pointed to; so a pointer point at an int is declared as "int *" while a pointer pointing at a class Widget is declared as "Widget *".



That's the gist of pointers, now the why. One very good reason is scope/lifetime.

Consider this function:

Player CreatePlayer()
{
Player p;
p.Name = "Bob";
// ... all kinds of other stuff
return p;
}


This code works perfectly fine, however when it returns, it actually creates a completely new copy of Player, so if the Player class is really large, this is going to be quite slow and wasteful. So, instead off going through the motions twice, you could:

Player* CreatePlayer()
{
Player * p = new Player();
p->Name = "Bob";
return p;
}

What happens here is CreatePlayer creates a new Player as a pointer instead of as a local variable. Since it is simply a pointer to a location in memory, you have to say "OK, now fill it with type Player, which is essentially what new does ( it actually does so by calling Player's constructor, but thats another story ). Now, since you are dealing with a player instead of a standard variable, you have to use -> instead of "." when accessing members. Now what made this call so much different than the earlier call? This is where scope comes into the equation.


Consider this:

{
int i = 42;
{
int j = 43;
}
int k = i + j;
}

Once wrong with this equation? Well, you can't access j because it went out of scope. That is exactly what the { } characters delineate. The lifetime of a variable is only as long as it is in scope. So once j hits the next } POOF, it ceased to exist.

So, what do you do when you want a variable to last longer than it's scope? This is where pointers can come in.

Consider this function again:


Player* CreatePlayer()
{
Player * p = new Player();
p->Name = "Bob";
return p;
}

At the end of this method, instead of a copy being returned, it's still the original p variable that exists, and the address of it is returned to the caller. This presents one of the biggest downsides to pointers too... leaks.

{
Player * ourPlayer = CreatePlayer();
// do stuff
}

That code just leaked a Player object, since variables created via pointers do not get destroyed when they go out of scope. Therefore you have to delete it yourself, such as:

{
Player * ourPlayer = CreatePlayer();
delete ourPlayer;
}


This has another downside, in that you need to be careful that a deleted pointer is never accessed again, consider:

{
Player * ourPlayer = CreatePlayer();
delete ourPlayer;
// stuff
cout << ourPlayer->Name;
}



KABOOOM!


Want to know the worst part... that's not even guaranteed to go KABOOM!, because even though you deleted the pointer, keeping in mind pointers are just a reference to a location in memory, even though you deleted the pointer, it doesn't mean that C++ has deleted what the pointer pointed to! That means that code may actually work, at least until something overwrites that location of memory. These create some seriously nasty and hard to find bugs! This is also why you will often see:

{
delete ourPlayer;
ourPlayer = NULL;
}

Because the guarantees that pointer will blow up if accessed again. Trust me, you would rather get a null pointer exception than have code that sorta works through that bug, so get in the habit of null'ing out your pointers.



Another big value of pointers is passing them around, lets say you had somethings like this:


{
Gameworld world;
ChangeWorld(world);
}

Gameworld ChangeWorld(Gameworld gameWorld)
{
gameWorld.time = "4:00pm";
// do other stuff to game world
return gameWorld;
}

Looks good and works, right? Well sorta. Here is the problem, you actually created 3! Gameworld objects in this process! The first one when you declared it, then when you passed it into ChangeWorld, since it was passed by value it actually created another local copy, then when you returned it, it created yet another copy to overwrite the original world object! This may be ok with small types like int, but with large classes or in a tight loop this is going to bring your program to a screeching halt.

Now, you can do the exact same thing with pointers:


{
Gameworld * world = new Gameworld();
ChangeWorld(world);
}

void ChangeWorld(Gameworld *gameWorld)
{
gameWorld->time = "4:00pm";
// do other stuff to game world
}

In this case, only one instance is ever created and even more importantly, you don't have to return the updated Gameworld object, as you actually made changes directly to the original.

That said C++ ( and not C ) provides a special quasi pointer called a reference.

When a reference is declared in a function, it changes things a bit. Generally when you are calling a function you are passing "by value", where as when you use a reference you are passing, predictably enough, by reference. You can think of this as the same as passing in a pointer, except the compiler does the work for you behind the scenes. So considering the earlier example, passed by reference it would be:


{
Gameworld world;
ChangeWorld(world);
}

void ChangeWorld(Gameworld &gameWorld)
{
gameWorld.time = "4:00pm";
// do other stuff to game world
}


So, it looks just like the first version, where it create a local copy, however because of that magical ampersand you are telling the compiler to pass in a reference to the object instead of a copy of the object, so in the function when you make changes, you are in fact making changes to the original.


Clear as mud?

Sorry it took me so long to respond. I don't mean to bring up an old thread. I would just like to say thank you for your very educational post. It took me at least 5 reads to grasp it but I finally understand pointers and implemented them in my game loop, the result is a much clearer and more optimized one that I'm very grateful for. Thanks again.

Went through the code today and cleaned it up a lot and commented it as much as I could to explain how everything works. Got the main source file game.cpp down to 96 lines.


// game.cpp
#include "Map.h"
#include "Player.h"
#include "Store.h"
#include "Display.h"
#include <cstdlib>
#include <iostream>
#include <fstream>
using namespace std;

int main()
{
////////////INITIALIZE GLOBAL VARIABLES////////////////////
//Initialize the game map, the player, and the display which contains functions used to display the various menus throughout the game..
//Also initialize an input variable to navigate the different menus of the game.
Map gameMap;
Player mainPlayer;
Display display;
int selection = 0;
////////////////////BEGIN START MENU/////////////////////
//Display the Start menu and prompt the user to load a previous game or start a new game.
display.Splash();
display.StartMenu();
cin >> selection;
cin.ignore();
if(selection == 2) {
//Load Game.
mainPlayer.loadGame();
gameMap.loadPlayerPos();
}
else {
//Create New Player
mainPlayer.createClass();
}
/////////////////////END START MENU /////////////////////////
/////////////////////////////////////////////////////////////
////////////////////BEGIN GAME LOOP//////////////////////////
/*Initialize control variable for main game loop.
If this variable is set true the game loop will exit.*/
bool done = false;
while( !done ) {
//Clear the screen.
system("CLS");
selection = 0;
// Each loop cycle we output the player position and a selection menu.
display.MainMenu(gameMap);
cin >> selection;
// Test the selection. If Selection == valid. Game Loop keeps going.
// If Selection != valid. Game loop restarts and prompts user for another selection.
if(!display.testSelection(selection, 1, 6))
continue;
//Initialize monster and set it to zero, in case the combat simulation is ran.
Monster* monster = 0;
switch( selection ) {
case 1://Player Moves
//Update players position on the game map.
gameMap.movePlayer();
/*If the Player has moved to the stores location on the game map they enter it.
Here the player can purchase weapons, items, and spells.
When the player is finished at the store he exits and returns to the game loop.*/
if( gameMap.getPlayerXPos() == 2 && gameMap.getPlayerYPos() == 3)
{
//Initialize store and enter it.
Store store;
store.enter(mainPlayer);
}
//If the player isnt at the stores location then test for a combat encounter.
else {
if(mainPlayer.combatSimulation( monster, mainPlayer, gameMap) >= 1)
done = true;
}
break;
case 2://Player Rests
if(mainPlayer.rest(monster, mainPlayer, gameMap) >= 1)
done = true;
break;
case 3://View Stats
mainPlayer.viewStats();
break;
case 4://View Map Landmarks.
mainPlayer.viewLandmarks();
break;
case 5://Save Game
mainPlayer.saveGame(gameMap.getPlayerXPos(), gameMap.getPlayerYPos());
break;
case 6://Quit
done = true;
break;
}
}
}
/////////////////END GAME LOOP///////////////////////////


Anymore comments or suggestions would be great. I'm still having some trouble with implementing the multiple enemies.
I'm not sure how I should go about making it balanced. If the player faces 3 enemies, should he have 3 attack turns? I'm just not sure.

An updated version of the game is attached to this post.
NOTE: The dungeon is not operational. It is part of a quest line I'm going to be implementing.

NPHshreddar, is it possible for you to put your project somewhere else? Because I can't download it for some reason (the first 5 downloads were my unsuccessful attempts).
With regards to the game.cpp you put on the forum, you probably shouldn't put srand in the game loop.

NPHshreddar, is it possible for you to put your project somewhere else? Because I can't download it for some reason (the first 5 downloads were my unsuccessful attempts).
With regards to the game.cpp you put on the forum, you probably shouldn't put srand in the game loop.

External Download Link

There ya go. Its hosted on fileswap.

Also, I removed the srand() function from the game loop and put it into the random function so it is called every time random is. Next feature is a quest line with some side quests. I'll be adding more map landmarks as well. I think I'm gonna be doing a redesign of the view map landmarks function to make more accurate and dynamic map. Ive also been considering consolidating all of the display functions for the store into a single function withtin the display class. We'll see.

You should not call srand inside the while loop. That will mess up the logic behind the random functions. Just call it once at the beginning of main.

If the player should get three turns against three enemies doesn't really make sense to me. If he got himself into a gang fight, he must deal with it

You could give the player a bonus turn if he does something smart or something, but these things is entirely up to you, the game creator

Also, I removed the srand() function from the game loop and put it into the random function so it is called every time random is.

Don't do this.
It should be called only once before you start pulling values from the rand function

You can reduce the repeated lines of code in the Map::checkRandomEncounter() function



Monster* Map::checkRandomEncounter()
{
int roll = Random(0, 20);

Monster* monster = 0;

if( roll <= 5 )
return 0;
else if(roll >= 6 && roll <= 10)
monster = new Monster("Orc", 20, 8, 200, 1, "Short Sword", 2, 7 , 20);
else if(roll >= 11 && roll <= 15)
monster = new Monster("Goblin", 12, 6, 100, 0, "Dagger", 1, 8, 10);
else if(roll >= 16 && roll <= 19)
monster = new Monster("Ogre", 28, 12, 500, 2, "Club", 3, 8, 45);
else if(roll == 20)
monster = new Monster("Orc Lord", 35, 15, 2000, 5, "Two Handed Sword", 5, 10, 200);

cout << "You encountered an " << monster->getName() << "!" << endl;
cout << "Prepare for battle!" << endl << endl;

return monster;
}


Maybe not a big deal but it reduces the chance of spelling errors

Your Player::rest function contains exactly the same logic as the combatSimulation. You should be able to reduce it to something like this:



int Player::rest(Monster* monster, Player& player, Map& map)
{
system("CLS");
cout << "Resting..." << endl;

mHitPoints = mMaxHitPoints;

return combatSimulation(monster, player, map);
}


That way, if you want to make some changes to the combatSimulation code, you have one place to do it, not two

If you add constructors to the Range and Weapon structs



struct Range
{
Range(int low=0, int high=0)
: mLow(low), mHigh(high) {}

int mLow;
int mHigh;
};



struct Weapon
{
Weapon() // Default constructor
: mName("Empty"), mDamageRange(Range()), mPrice(0) {}

Weapon(const std::string& name, const Range& range, int price) // Parameterized constructor
: mName(name), mDamageRange(range), mPrice(price) {}

std::string mName;
Range mDamageRange;
int mPrice;
};

Then you should be able to get away with this in the Store constructor



Store::Store()
{
//Initialize Weapons.
mWeapons.push_back(Weapon("ShortBow", Range(4, 6), 200));
mWeapons.push_back(Weapon("Scimitar", Range(3, 8), 250));
mWeapons.push_back(Weapon("BattleAxe", Range(2, 9), 300));
mWeapons.push_back(Weapon("Halberd", Range(1, 10), 350));
mWeapons.push_back(Weapon("BastardSword", Range(5, 10), 500));
....
}


You should be able to do the same thing with Items and Spells

You have a couple of these


cout << "ERROR: You don't have enough gold for that item!" << endl;


Not having enough gold is really not an error...

You do this a lot:


int input = 0;
cin >> input;


This is a ticking bomb. If we type something invalid at the console this will halt our program. Very annoying for the game player.

I would make a bullet proof function specifically for this purpose


int getUserInput()
{
int input;
std::string line;
while(true) // Continue until we have a valid integer...
{
std::getline(std::cin, line); // Read a string. That way we can handle anything without crashing
std::stringstream ss(line); // Move the line into a stringstream object
if(ss >> input) // Convert the line into a integer
break; // The conversion was a success, end the loop
else
std::cout << "Invalid input: " << line << ". Try again" << std::endl;
}
return input;
}

I haven't tested this function but something along those lines should work. It requires the string, iostream and sstream headers.





The class name Player is a bit ambiguous. Is a player the guy playing the game or is it the character he is playing?
Maybe Character is a better name for this class.





Finally, I would consider making the different character races into separate classes inheriting from the character class.
This is a somewhat advanced feature though so I won't recommend you to do this unless you are familiar with inheritance and virtual functions.

Wow, a lot of really great suggestions there, very much appreciated.
I've gone through and made quite a few large changes to the code, ill post an updated version when it's completed.

I'm a little stuck on implementing the getUserInput function though. First of all the program still behaves incorrectly on an invalid value. Say the menu has 4 options, I enter 5 as my input it passes 5 back and something unexpected happens like moving in a random direction, the wrong weapon is bought, or the wrong item is used, etc.

Secondly, whenever I go to the battle menu and use this function it immediately without any input displays invalid input . try again. I think it has to do with the newline character left in the input buffer from the previous menu after we hit enter. Having a hell of time trying tot fix this though.

Basically I need Input function for integers that takes two parameters. A low value and a high value. The users input can not be lower than low or higher than high, if it is then the function should re-prompt them for input.

Then I will need a second input function for Strings like the players name.

Wow, a lot of really great suggestions there, very much appreciated.
I've gone through and made quite a few large changes to the code, ill post an updated version when it's completed.

I'm a little stuck on implementing the getUserInput function though. First of all the program still behaves incorrectly on an invalid value. Say the menu has 4 options, I enter 5 as my input it passes 5 back and something unexpected happens like moving in a random direction, the wrong weapon is bought, or the wrong item is used, etc.

Secondly, whenever I go to the battle menu and use this function it immediately without any input displays invalid input . try again. I think it has to do with the newline character left in the input buffer from the previous menu after we hit enter. Having a hell of time trying tot fix this though.

Basically I need Input function for integers that takes two parameters. A low value and a high value. The users input can not be lower than low or higher than high, if it is then the function should re-prompt them for input.

Then I will need a second input function for Strings like the players name.

Yes you are right. Mixing cin >>, and getline(cin, ...) causes all kinds of problems.

I managed to make a function that might do better in this situation, but more testing is needed (I don't trust it yet)
Also, reading a string from user had to become a separate function because of the arguments



template<class T>
T getUserNumber(T low, T high)
{
T t;
std::cin.sync();
while(!(std::cin >> t) || t < low || t > high)
{
std::cin.clear();
std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
std::cout << "Invalid input. Try again" << std::endl;
}
return t;
}

std::string getUserString()
{
std::string s;
std::cin.sync();
std::getline(std::cin, s);
return s;
}


I made the number function a template so it can be used to read doubles. You can remove this and make it a int only function if you want

Here is my test



int main()
{
using namespace std;
int i;
double d;
string s;

cin >> i;
cout << i << endl;

cout << "Enter a string: ";
s = getUserString();
cout << s << endl;

cout << "Enter a integer: ";
i = getUserNumber<int>(1, 3);
cout << i << endl;

cout << "Enter a decimal: ";
d = getUserNumber<double>(1.1, 3.3);
cout << d << endl;
}


I showed you a Range constructor earlier taking two arguments (low, high)
This constructor allows us to provide 0, 1, or 2 arguments:


Range() // 0 -> 0
Range(5) // 5 -> 0
Range(1, 5) // 1 -> 5


As you can see, the constructor taking only one argument always results in a useless range.
We can fix this by giving the Range class three constructors instead



struct Range
{
Range() : mLow(0), mHigh(0) {}
Range(int high) : mLow(0), mHigh(high) {}
Range(int low, int high) : mLow(low), mHigh(high) {}

int mLow;
int mHigh;
};



Range() // 0 -> 0
Range(5) // 0 -> 5
Range(1, 5) // 1 -> 5


Just a little nitpicking...

I spent quite a few hours working out some major bugs yesterday.. Finally arrived at this for the input of integers. It gets the job done and is relatively simple. Had to give it unsigned int for the types so I could compare the input integer to the size of a vector. Your string input function works so far, although i haven't used it more than once in the program. The info about the Range constructor has been noted.


unsigned int Player::getUserInput(unsigned int low, unsigned int high)
{
unsigned int input = 0;
while(true) // Continue until we have a valid integer...
{
cout << "Selection: ";
cin >> input; // Read a string. That way we can handle anything without crashing
cin.ignore(); // Clear input buffer.
if(input >= low && input <= high) // Test if integer is within the bounds.
break; // The integer is valid, end the loop.
cout << "Invalid input: " << input << ". Try again" << endl;
}
return input;
}


Also thanks to your suggestions the store code is a lot more simplified and easier to manage.


// Store.cpp
#include "Store.h"

Store::Store()
{
//Initialize Weapons.
mWeapons.push_back(Weapon("ShortBow", Range(4, 6), 200));
mWeapons.push_back(Weapon("Scimitar", Range(3, 8), 250));
mWeapons.push_back(Weapon("BattleAxe", Range(2, 9), 300));
mWeapons.push_back(Weapon("Halberd", Range(1, 10), 350));
mWeapons.push_back(Weapon("WarHammer", Range(5, 7), 450));
mWeapons.push_back(Weapon("BastardSword", Range(5, 10), 500));
//Initalize Items.
mItems.push_back(Item("HealingPotion", "Healing", Range(0, 0), Range(6, 8), Range(0, 0), 20));
mItems.push_back(Item("MagicPotion", "MagicRestore", Range(6, 8), Range(0, 0), Range(0, 0), 20));
mItems.push_back(Item("FireBallPotion", "Damage", Range(0, 0), Range(0, 0), Range(6, 8), 30));
//Initalize Spells.
mSpells.push_back(Spell("MagicMissle", "damage", Range(3, 6), Range(0, 0), 4, 200));
mSpells.push_back(Spell("MinorHeal", "heal", Range(0, 0), Range(4, 7), 4, 250));
mSpells.push_back(Spell("Absorb", "hybrid", Range(5, 5), Range(5, 5), 5, 350));
mSpells.push_back(Spell("ChainLightning", "damage", Range(6, 12), Range(0, 0), 6, 450));
mSpells.push_back(Spell("MeteorSwarm", "damage", Range(8, 10), Range(0, 0), 6, 500));
mSpells.push_back(Spell("FireBall", "damage", Range(4, 5), Range(0, 0), 6, 500));
mSpells.push_back(Spell("MajorHeal", "damage", Range(0, 0), Range(8, 10), 5, 750));
}

//Display the store and its contents and allow the player to browse the store and purchase items until they choose to exit.
void Store::enter(Player& player, Disp& disp)
{
//intialize variables for use in the store function.
bool done = false;
//Continue to display the store until the player is done.
while(!done)
{
//Clear the screen.
system("CLS");
//Display the main store menu.
disp.StoreMainMenu();
//Test for invalid selection.
int selection = player.getUserInput(1, 4);
//Display sub-menu based on user's selection.
switch(selection)
{
case 1://Buy Weapons.
{
disp.StoreWeaponMenu(mWeapons, player);
int s = player.getUserInput(0, mWeapons.size());
//Player buys the weapon if the selection was valid and the player has enough gold.
if(mWeapons[s].mPrice <= player.getGold())
player.buyWeapon(mWeapons[s]);
//Player doesnt have enough gold and returns to the main store menu where they may exit.
else
cout << "You dont have enough gold for that weapon!" << endl; system("PAUSE");
//function has completed return to store main menu.
break;
}
case 2://Buy Items.
{
disp.StoreItemMenu(mItems, player);
//Test for invalid selection.
int s = player.getUserInput(0, 2);
//Player buys the item if the selection was valid and the player has enough gold.
if(mItems[s].mPrice <= player.getGold())
player.buyItem(mItems[s]);
//Player doesnt have enough gold and returns to the main store menu where they may exit.
else
cout << "You don't have enough gold for that item!" << endl; system("PAUSE");
//function has completed return to store main menu.
break;
}
case 3://Buy Spells.
{
disp.StoreSpellMenu(mSpells, player);
//Test for invalid selection.
int s = player.getUserInput(0, mSpells.size());
//Player buys the spell if the selection is valid and they have enough gold.
if(mSpells[s].mPrice <= player.getGold())
player.buySpell(mSpells[s]);
//Player doesn't have enough gold and is returned to the main store menu where they may exit.
else
cout << "You dont have enough gold for that spell!" << endl; system("PAUSE");
//function has completed return to store main menu.
break;
}
case 4://Exit Store.
{
//Set done to true. This will cause you to return out fo the store function back to the main game menu.
done = true;
break;
}
}
}
}


I'm going to try to get the player class looking this good. Its going to be quite the task though as it is almost 1,000 lines of code.

Once again, thanks for your suggestions, I'll be posting an updated version with more changes very soon.

Thats great

edit:
I posted a different version of the input function above, if you didn't notice.
If you get any problems you could check them out