# Text-based adventure game with too many conditional statements

For a school project, I am creating a text-based adventure game in C++. I have shown my code to my teacher and he is stating my code is not entirely logical in sequence and that I should make my if statements inside functions because there are so many. I need help as to know how to make these changes.

Note that this is one function of the code, and not it's entirety.

void road() {
int choice2;
int choice3;
int event1;
system("cls");
cout << "You are on a road that heads west and east of your position." << endl;
cout << "Which way will you go?\n" << endl;

cout << "1. Go West" << endl;
cout << "2. Go East" << endl;
cout << "3. Wait for something to happen\n" << endl;

cin >> choice2;
cin.ignore();
system("cls");

if (choice2 == 1) {
int decision1;
cout << "You travel down the road, about only 100 metres and you encounter " << endl;
cout << "a giant spider with vicious poison coated fangs." << endl;
cout << "its hideous appearance causes your throat to dry and your knees to shake!" << endl;
cout << "What on earth will you do?\n\n" << endl;
cout << "1. Attempt to attack the spider with you sword." << endl;
cout << "2. Throw your sword in the off chance it might kill it." << endl;
cout << "3. RUN FOR YOUR LIFE!" << endl;

cin >> decision1;
cin.ignore();
system("cls");

if (decision1 == 1) {
cout << "You viscously swing your sword at the spiders general direction." << endl;
cout << "The swing was so great, your arms jolts out of place, \ncreating a surge of pain." << endl;
cout << "Your arm is now broken, and you fall to the ground in pain...." << endl;
cout << "The spider launches 3 metres straight into your body... \nWhat on earth is it doing?" << endl;
cout << "Oh My God! The spider is devouring everything...." << endl;
cout << "All that remained was bones of the once mobile adventurer " << name << endl;
system("pause");
gameover();
}
else if (decision1 == 2) {}
else if (decision1 == 3) {}

}
else if (choice2 == 2) {
cout << "After a mile walk, you arrive at an old brick house. " << endl;
cout << "You walk slowly inside." << endl;
cout << "The door slams behind you and the room lightens up." << endl;
cout << "What on earth is going on...?\n\n" << endl;
system("pause");
cout << "\nUnable to open the door, you look around for anything of use." << endl;
cout << "Nothing, not a single piece of furniture." << endl;
cout << "What will you do?\n" << endl;
cout << "1. Wait for someone to save you." << endl;
cout << "2. Or Wait for someone to save you." << endl;

cin >> choice3;
cin.ignore();
system("cls");
if (choice3 == 1) {
cout << "As you character relaxes his legs and begins to sit" << endl;
cout << "he leans against the wall, only to hit a hidden switch." << endl;
system("pause");
cout << "The floor beneath you disappears, and you start to plummet." << endl;
cout << "What a trap! You bloody stupid fool!\n\n?" << endl;
cout << "somehow you never reach the bottom....." << endl;
cout << "You die of water deprivation, after only 2 weeks of falling...." << endl;
system("pause");
gameover();
}
else if (choice3 == 2) {
system("cls");
cout << "After only 5 hours..." << endl;
cout << "The doors suddenly swoosh open." << endl;
cout << "You are free!  " << endl;
system("pause");
cout << "You take your first steps into what is now moonlight," << endl;
cout << "and then suddenly pass out on the ground...." << endl;
cout << "For some peculiar reason or another, you are now /nback in the creepy dark and gloomy library. You swiftly escape its nostalgia. " << endl;
}

}

else if (choice2 == 3) {
int random1;
cout << "After at least was seemed to be 4 hours " << endl;
cout << "a dragon swoops down from the heavens...." << endl;
cout << "and lands right in front of you!\n" << endl;
system("pause");
system("cls");

random1 = rand() % 2; // pseudo random generator
if (random1 == 0) {
int choice4;
cout << "Your character screams at the top of his lungs, " << endl;
cout << "this causes the dragon to immediately to bow in fear..." << endl;
cout << "It turns out dragons are very sensitive to hearing....." << endl;
system("pause");
cout << "\nIt seems the dragon is requesting you ride it!\n" << endl;
cout << "Will you ride it?\n" << endl;
cout << "1. Ride it" << endl;
cout << "2. Or Wait here." << endl;

cin >> choice4;
cin.ignore();
system("cls");

if (choice4 == 1){
Ending();
}
else if (choice4 == 2){
}

}
else if (random1 == 1) {
cout << "Your character screams at the top of his lungs, " << endl;
cout << "eventually your breath gives out and you die because of lack of oxygen." << endl;
system("pause");
gameover();
}

}
}

-
(1) Don't make function because there are too many lines of code but group functionality in functions. (2) Don't use numeric literals in code but use constants/enums with descriptive names. –  harper May 13 at 8:02
Refactor. Start by trying to move the code inside each choice into a function. Than refactor further to move common functionality into separate smaller functions. It's much like sculpting. –  StoryTeller May 13 at 8:07
you shoudl probably have a decision class/method that contains the switch logic, the actual implementation of the logic in the decision should be in its own methods (and perhaps even class depending on how far you want to go with modelling your domain language in classes). I don't know about c++ but in c# lots of if statements are considered a code smell in some camps. Have a read about state machines you might find them a good pattern for your problem ;) –  John Nicholas May 13 at 8:36
Not an answer to your question, but you might consider learning about the programming language Inform7 if you're interested in writing your own text adventures. –  Eric Lippert May 13 at 16:07
On a side note, there's a huge difference between viscously and viciously. Sorry, pet peeve. –  cjm May 13 at 20:33

## Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. I don't know that you've actually done that, but it's an alarmingly common thing for new C++ programmers to do.

## Avoid the use of global variables

I see that name is used but it's neither declared within the function nor passed to it, implying a global variable. It's generally better to explicitly pass variables your function will need rather than using the vague implicit linkage of a global variable.

## Eliminate unused variables

This code declares a variable event1 but then does nothing with it. Your compiler is smart enough to help you find this kind of problem if you know how to ask it to do so.

## Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++.

The string starting with "For some peculiar reason" contains /n which is two characters, but it's clear from the context that you intended \n which is a single newline character.

## Use a menu object or at least a common menu function

In a number of places in your code, you have something like a menu. Your code presents a couple of options and then asks the user to pick one based on an input number. Rather than repeating that code in many places, it would make sense to make it generic. Only the prompt strings actually change, but the underlying logic of presenting the choices and asking for input are all the same. It looks like you're a beginning programmer, and so perhaps you haven't learned about objects yet, but this kind of repeated task with associated data is really well-suited to object-oriented programming and that's something that C++ is very good at expressing.

Specifically, here's how I might approach this. I'd create a Menu object:

class Menu
{
public:
Menu(const string &name, const string &prompt,
const std::vector<std::pair<string, string> > &choices
= std::vector<std::pair<string, string> >{});
const string& getChoice() const;
bool operator==(const string &name) const;
private:
string _name, _prompt;
std::vector<std::pair<string, string> > _choices;
};


Implementations are here:

Menu::Menu(const string &name, const string &prompt,
const std::vector<std::pair<string, string> > &choices)
: _name(name), _prompt(prompt), _choices(choices)
{}

{
return name==_name;
}

{
if (_choices.size() == 0) {
cout << _prompt;
}
unsigned choice;
int i;
do {
cout << _prompt;
i = 1;
for (auto ch : _choices)
cout << i++ << ": " << ch.first << '\n';
cin >> choice;
--choice;
} while (choice >= _choices.size());
return _choices[choice].second;
}

{}



Finally, we can construct the game itself as a std::vector of these Menu objects:

std::vector<Menu> game{
"Which way will you go?\n", std::vector<std::pair<string,string> >{
{"Go West", "spider"},
{"Go East", "brickhouse"},
{"Wait for something to happen", "dragon"}}),
"You travel down the road, about only 100 metres and you encounter \n"
"a giant spider with vicious poison coated fangs.\n"
"its hideous appearance causes your throat to dry and your knees to shake!\n"
"What on earth will you do?\n\n", std::vector<std::pair<string, string> >{
{"Attempt to attack the spider with your sword.","spiderattack"},
{"Throw your sword in the off chance it might kill it.","throwsword"},
"You viscously swing your sword at the spiders general direction.\n"
"The swing was so great, your arms jolts out of place,\n"
"creating a surge of pain.\n"
"Your arm is now broken, and you fall to the ground in pain....\n"
"The spider launches 3 metres straight into your body...\n"
"What on earth is it doing?\n"
"Oh My God! The spider is devouring everything....\n"
"All that remained was bones of the once mobile adventurer.\n"),
"After a mile walk, you arrive at an old brick house.\n"
"You walk slowly inside.\n"
"The door slams behind you and the room lightens up.\n"
"What on earth is going on...?\n\n"
"Unable to open the door, you look around for anything of use.\n"
"Nothing, not a single piece of furniture.\n"
"What will you do?\n", std::vector<std::pair<string, string> >{
{"Wait for someone to save you.", "trapdoor"},
{"Or Wait for someone to save you.", "library"}})
};


The game itself becomes entirely data-driven:

void road() {
}


If you need more than the plain Menu class provides, you can simply derive a new kind of class and put that into the vector. It should also be obvious that this could very easily all be read as a script from a file, simply by defining an ostream extractor for the Menu class.

## Consider using a better random number generator

You are currently using

random1 = rand() % 2;


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

int rand_int(int low, int high)
{
static std::default_random_engine re {};
using Dist = std::uniform_int_distribution<int>;
static Dist uid {};
return uid(re, Dist::param_type{low,high});
}


Note that this still stores state, but at least the distribution is correct.

-
So show an example of usage of std::uniform_int_distribution –  Loki Astari May 13 at 14:54
@LokiAstari: good point. I've edited my answer to include an example usage of std::uniform_int_distribution from a credible source. –  Edward May 13 at 15:48
I feel that this answer does a lot of nitpicking, while not addressing the central concern of the design of the program. –  Gustav Bertram May 14 at 8:27
@GustavBertram: I've added a detailed example of the menu object idea to attempt to address your concern. –  Edward May 15 at 18:44
Looks much better! –  Gustav Bertram May 16 at 14:35

I guess what your teacher meant is this:

if (choice == 1) {
spider();


But there is a better way to write this. You adventure game is actually a Finite State Machine. You could implement it with a simple loop:

#include <iostream>

struct  state;
struct  transition;

struct transition {
char *text;
struct state *next_state;
};

struct state {
char *text;
struct transition transitions[8];
};

extern struct state start;
extern struct transition start_transitions[];
extern struct state spider;
extern struct transition spider_transitions[];

struct state start = {
"Which way will you go?",
{
{"Go West", &spider},
{"Go East", NULL},
{"Wait for something to happen", NULL},
{ NULL }
}
};

struct state spider = {
"You travel down the road, about only 100 metres and you encounter\n"
"a giant spider with vicious poison coated fangs.\n"
"Its hideous appearance causes your throat to dry and your knees to shake!\n"
"What on earth will you do?",
{
{ "Attempt to attack the spider with you sword.", NULL },
{ "Throw your sword in the off chance it might kill it.", NULL },
{ "RUN FOR YOUR LIFE!", NULL },
{ NULL }
}
};

int main(void)
{
state *cur = &start;
while (cur) {
// Print text
std::cout << cur->text << std::endl;

// List available choices
unsigned trans = 0;
while (cur->transitions[trans].text) {
std::cout << trans << ". " << cur->transitions[trans].text << std::endl;
trans += 1;
}

unsigned choice;
std::cin >> choice;
std::cin.ignore();

// Check input is valid
if (choice < trans) {
// Move to new state
cur = cur->transitions[choice].next_state;
}
}
return 0;
}


Of course, a more mature version of the game would read the states and transitions from a data file rather than include them directly in the code.

-
pff you just removed the discovery of finite state machines. Its fun inventing this stuff yourself then seeing the actual patterns –  John Nicholas May 13 at 13:34
There are designated initializers, which are C99, not C++. Are you using a compiler extension (clang)? –  Morwenn May 13 at 13:47
@Morwenn You are right. I write C code most of the time. I've mostly stopped using C++ 15 years back. I will remove the C99 designated initializers. And I use compilers (CLang and GCC) which support this. –  Erwan Legrand May 13 at 16:16
@John Nicholas Perhaps I did. More likely, the OP would have learned about it as part of a course or by reading someone else's code. –  Erwan Legrand May 13 at 16:25

I would say we need to look at design of your game.

To me an adventure game is moving through a set of locations. At each locatoin you can move to another location (that is linked to the current location) or interact with objects at that location. So it seems to me that you want to build a graph of locations.

So first we have a game object.

 class Game
{
std::string  currentLocation;                // Assuming a one player game
// You only need to store that
// players location.

std::map<std::string, Location>   locations; // All locations in the game
// You can look them up by name.
};


So we have a game with locations. But how does that fit together.

 class Location
{
std::string                           description;
// here. The action is the key
// the location is the value
// and you can get the location
// details by looking up the
// location in the map.
std::vector<std::string>              thingsLyingHere;
};

Example:
========
linked["Wait for something to happen"] = "Waiting At Start";


Now your main program looks at your current location. Looks that location up in the Game object and can print out the description and options for that location. When your user types in one of the available actions you look that up in linked and update the current location. Then you start the loop again.

-

Your code does a very poor job in separating code from data. I would suggest you split your projects into two parts: a data file to define your adventure and code which parses the data file and lets you play the game.

The following is a suggestion how it might work. You can come up with your own more sophisticated "grammar" for the data file. There are also tools around to do the parsing for your, but as this project is for learning, I would suggest your write your own parsing code.

If you want some custom code to run, like doing a fight or rolling a random choice, you can also add that to your grammar and link tags to functions you want to execute (for example using if statements to select the correct piece of code).

Have fun!

### Data file

A normal text file, e.g. "the_dragon_tale.txt", which has all your text messages and the possible choices. The text file is annotated to indicate the program flow.

Example:

[road]
Which way will you go?
#west Go West
#east Go East
#roadwait Wait for something to happen

[west]
A spider!!
#fightspider

[east]
A dragon!
#endofgame

Very boring.


### Code

Your program now looks completely different. First write a function which reads and parses your adventure text file. For each [tag] section it creates an instances of a class and links and the possible choices marked by the # sections.

Example:

struct Choice {
std::string text;
std::string target;
}

struct Encounter {
std::string tag;
std::string message;
std::vector<Choice> choices;
}

/** check online resources */
}

Encounter ParseEncounter(const std::string& text) { ... }

std::map<std::string,Encounter> ParseEncounters(const std::string& text) {
std::map<std::string,Encounter> encounters;
size_t pos = 0;
while(true) {
// find next [
size_t next = text.find("[");
if(next == std::string::npos) break;
// get the encounter text
std::string enc_str = text.substr(pos,next);
// parse encounter
Encounter enc = ParseEncounter(enc_str);
// store encounter
encounters[enc.tag] = enc;
// next
pos = next;
}
return encounters;
}

std::string RunEncounter(const std::map<std::string,Encounter>& encounters, const std::string& tag) {
system("cls");
// get encounter
Encounter node = encounters[tag];
// print message and choices
std::cout << node.message << std::endl;
for(size_t i=0; i<node.choices.size(); i++) {
std::cout << i << ". " << node.choices[i].text << std::endl;
}
// get correct choice from user
int choice = -1;
while(!(0 <= choice && choice < node.choices.size())) {
std::cin >> choice;
}
cin.ignore();
// return choice tag
return node.choices[choice].target;
}

int main() {
std::map<std::string,Encounter> encounters = ParseEncounters(str);
while(tag != "endofgame") {
tag = RunEncounter(tag);
}
}

-
+1 for completeness, though given the nature of the question I was inclined to leave some work for the OP, given that they appear to be doing some sort of homework assignment ;) –  Rook May 13 at 8:35
There is still a lot left to do, and I would be surprised if I did not make a mistake somewhere ;) –  Danvil May 13 at 14:23

I'd start breaking your code up into chunks. There are a couple of ways you could do this, such as by giving each room its own function (and I can remember doing this with my first text-adventure type game, way back when!) or alternatively by defining a room as a data structure, like this:

struct Choice
{
int nextroom;
std::string text;

Choice(int n, std::string t) : nextroom(n), text(t) {}
};

struct Room
{
const std::string text;
std::vector<Choice> choices;
bool deathroom;

Room(std::string t, std::vector<Choice> c, bool d = false)
: text(t), choices(c), deathroom(d) {}
};


Now, you can define a collection of rooms:

std::vector<Room> rooms;
Room r1(
"You are in room 1.\n",
std::vector<Choice>{ Choice(2, "Option 1"), Choice(3, "Option 2") });
rooms.push_back(r1);


With this, you can turn your game into a single loop:

int room = 0;
while (rooms[room].deathroom == false)
{
std::cout << rooms[room].text;
room = get_choice(room);
}
std::cout << "Game Over!\n";


You'll need to implement get_choice yourself, of course ;)

This makes it much simpler to add and remove rooms, or even in the future read room definitions from a text file, and so on.

-
Ah, you came up with a similar solution. I think this is a good approach! In my post I also suggested to use a data file to define the rooms. –  Danvil May 13 at 8:33

I agree with your teacher that breaking the code into functions will help the readability of your code.

For example, put the code, for the result of each decision, in its own function.

cout << "1. Go West" << endl;
...

if (choice2 == 1) {
go_west();
} else if (choice2 == 2) {
go_east();
} else if (choice2 == 3) {
wait();
}


If I am reading the code this makes it much easier to figure out what is happening. What happens if I input 2? I do not need to skip over all the go west or wait for something to happen code because they are in functions somewhere else. If I am interested in that code then I look at the function.

This also helps if two routes lead to the same result, you do not need to copy and paste the code you simply call the same function.

If you have learned about switch statements then it might get you some more points to use a switch instead of if/else.

switch (choice2) {

case 1:
go_west();
break;
case 2:
go_east();
break;
case 3:
wait();
break;
}


This is not very important in the real world though. Use whichever you think looks better.

It would also be an idea to use enums or constant values when processing the decision points. If I look at the if/else statements I need to remember: does 1 mean east or west?

#define WEST 1
#define EAST 2
#define WAIT 3

if (choice2 == WEST) {
go_west();
} else if (choice2 == EAST) {
go_east();
} else if (choice2 == WAIT) {
wait();
}


A more advanced strategy would be to use a jump table.

void(*tbl[4])(void) = {
&invalid_input,
&go_west,
&go_east,
&wait
};

cin >> choice2;

if (choice2 > 3)
choice2 = 0;

tbl[choice2]();


This may not be a viable option for you at your current stage but it might help other people or yourself later on in your education.

-
Using defines in this way is probably a bad idea. Each location has a different set of answers, and defines are pretty global. –  Gustav Bertram May 14 at 8:31

A good general rule is that if the code takes more than two screens of text, it should be split into functions. Sure you can read your spaghetti code, but always remember that you're coding for other people to read it. These people may be your teacher (at school), your co-workers (when you leave school), or you in 6 months time (when you look at it and go "WTF?!").

The big thing with functions though is that you may have more than one way of getting somewhere. If you're on a flat featureless plain and you go north then west, you'd usually expect to get to same place as if you go west then north. Your code simply doesn't allow this to happen though. Do you want to copy-and-paste code to do the same thing twice? (If your answer is anything other than variations on the theme of "hell no!" then you've just failed your programming aptitude test. ;) Functions will let you do this very easily, so use them.

One thing in your code that you want to fix is re-entrant function calls. What I mean by this is that you're calling road() from within road(). This is almost always a bad thing to do. If there are any changes to global resources during the function (hint: cls() changes the state of a global resource...) then your re-entry may do things you didn't want it to. You'd need to keep track of how each re-entry affects the ones before it, which gets epicly complex very quickly. Worse, each time you re-enter, you use up a new chunk of stack space. On really small systems (like Arduino) you can easily use up more memory than the system has free, and it all crashes. Even on big systems like PCs, if you accidentally set up something which re-enters in a loop and keeps going for a while then you can kill things that way too. You really need to fix this, because it's very bad practice.

Oh, and have a play of some of the classic 80's text adventure games. "Planetfall" or the "Zork" series are top tips.

-
Good point on the re-entrant function calls. I missed that one! –  Edward May 13 at 15:39

An important rule of thumb when programming is to make function blocks small enough to fit entirely on the screen, let's say, no more than about 20 lines of code. Otherwise you easily lose overview when writing really big programs.

So, split your code into chunks you could give a meaningful name, put each chunk into a subfunction you give just that name and replace the code chunk in your original function by just calling that subfunction. In case any local variables are needed in your code chunk pass them as parameters of your subfunction and return any values needed further.

-
this approach is not in favour of performance (in high end performing application). Better to use #pragma or code blocks {} to organize your function implementation rather than spliting them into meaningless functions. having many functions in code might result in "spaghetti" code - where reading through it will be come very tedious –  NirMH May 13 at 8:14
Indeed, instead of limiting the length of a function, you should make sure a function does one thing. Often, increasing line count by adding whitespace and good formatting increases readability and splitting everything off into too many functions decreases a reader's comprehension of the code. –  rubenvb May 13 at 8:16

Grouping functionality in functions would be a first step. You could add functions like that:

void printStoryText(std::string storyText) { /* impl */ }

int decide(std::string decisionText,
const std::vector<std::string>& choices) { /* impl */ }


Put the cout and pause statements in the printStoryText. Decisions should be done with the decide function which accepts a string containing the decision description and a vector of strings with the possible choices. It returns the index of the chosen element.

Add individual functions for each step of your story and call them as result of the decision outcome. Make separate functions for the brick house, the dragon, etc.

Of course this strategy won't be sufficient as your adventure game grows bigger. I would suggest to abstract the game logic into a data structure suitable for that task. A tree would be the best choice I think. The following code samples should illustrate the idea, they are not complete (most importantly they are missing most of the public interface):

struct Choice
{
std::string choice_text;
std::shared_ptr<DecisionNode> next_node;

Choice(const std::string& choice, std::shared_ptr<DecisionNode> next)
: choice_text(choice)
, next_node(next)
{}
};

class DecisionNode
{
public:
DecisionNode(const std::string& text)
: story_text_(text)
{}

void AddChoice(const std::string& text, std::shared_ptr<DecisionNode> nextNode)
{
child_nodes_.push_back(Choice(text, nextNode));
}

std::shared_ptr<DecisionNode> Decide() const
{
std::vector<Choice>::const_iterator it = choices_.begin();
for(unsigned int i = 1; it != choices_.end(); ++it, ++i)
{
std::cout << i << ". " << it->choice_text << std::endl;
}

unsigned int selected;
std::cin >> selected;

return choices_.at(selected);
}

private:
std::string story_text_;
std::vector<Choice> choices_;
}


You would then build a tree first before actually starting the game logic by creating a root node with the top level story and the following decision. Each choice of a decision has a choice text and a next node which will be selected when chosen. The Decide() function returns the next node. This will absorb the many if/else statements that you would need otherwise into the tree structure.

-
Better to use shared_ptr than raw pointers, for next_node. –  Rook May 13 at 8:57
Yes I agree. I updated the sample. –  jasal May 13 at 9:00