HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMajor

The Mysts of Altair - text-based adventure game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thetextadventurealtairgamebasedmysts

Problem

This is my code for The Mysts of Aesthoth so far. I'd like to know if I'm following best practices, and what can be improved.

```
/*
______________________________________
This Program was made by: Andrew Tew
______________________________________

*/

#include
#include
using namespace std;

int main()
{
//EVERYTHANG IN DER BACKGRUND

int Charma = 0;
unsigned int Hunger = 10;
unsigned int Energy = 50;
unsigned int Health = 100;

string Ans1;
string Ans2;
string Ans3;
string Ans4;
string Ans5;
string Ans6;
string Ans7;
string Ans8;
string Ans9;
string Ans10;
string Ans11;
string Ans12;
string Ans13;
string Ans14;
string Ans15;
string Ans16;
string Ans17;
string Ans18;
string Ans19;
string Ans20;
string Ans21;
string Ans22;
string Ans23;
string Ans24;
string Ans25;
string Ans26;
string Ans27;
string Ans28;
string Ans29;
string Ans30;
string AnsCharSure;

string eyecolor;
string skintone;
string haircolor;
string height;
string name;
string race;
string gender;

bool dead = false;
bool human = false;
bool orc = false;
bool goblin = false;
bool elf = false;
bool lizard = false;
bool cat = false;
bool vampire = false;
bool werewolf = false;
bool SNK = false;
bool Get_Backpack = false;

StartScreen:
system ("cls");
system ("color 0c");
system ("title Mysts of Altyar");
cout > Ans1;
cout > race;
if (race == "lizard"){
lizard = true;
goto SkinTone;
}
if (race == "elf"){
elf = true;
}
if (race == "orc"){
orc = true;
}
if (race == "SupahNinjaKitty"){
SNK = true;
}
if (race == "cat"){
cat = true;
}
if (race == "goblin"){
goblin = true;
};
if (race == "human"){
human = true;
}
Gender:
cout > gender;
HairColor:
cout > haircolor;
SkinTone:
cout > skintone;
EyeColor:
cout > eyecolor;
Height:
if (goblin = true){
goto Name;
}
cout > height;
if (haircolor == "black" && skintone == "white" && eyecolor == "red" && height == "tall"){
vampire = true;
}
if (haircolor == "brown" && skintone == "brown" && eyecolor == "brown" && height == "tall"){

Solution

There's a lot to comment on here, so I'll try to cover everything while being brief.

Encapsulation

int Charma = 0;
unsigned int Hunger = 10;
unsigned int Energy = 50;
unsigned int Health = 100;


Presumably, these are all attributes of a player. That suggests that you want to stick them all in a type:

struct Player {
    int Charma = 0;
    unsigned int Hunger = 10;
    unsigned int Energy = 50;
    unsigned int Health = 100;
};


and have an instance of Player, instead of just referencing variables. Reading further, there are other attributes that correspond to the player's character that should also get added.

That's a LOT of variables

At some point while declaring all your string Ans*, you should pause to realize that this is excessive. You only ever need one of these at a time, and you only ever need them for a local instance. That is, all of these variables are unnecessary. You need to separate your one giant function into small pieces.

Since a lot of your questions are yes/no, that strongly suggests a:

bool prompt(std::string const& message) {
    std::string answer;
    std::cout > answer;
    if (answer == "yes") {
        return true;
    }
    else if (answer == "no") {
        return false;
    }
    else {
        throw std::runtime_error("fail"); // or keep looping until
                                          // the user enters something valid
                                          // but definitely do SOMETHING
    }
}


Hence:

bool get_backpack = prompt("Get the backpack?\n");


And we no longer have name pollution with cryptic names. get_backpack is a lot easier to understand than Ans11.

Goto

At a first approximation, never use goto. It makes it more or less impossible to follow code. Without goto, you also don't need labels. Instead, just call different functions. Or keep an internal state machine. Or really any possible other solution. Code with goto like this is incredibly brittle - what if you want to add more states somewhere in between? Good luck keeping track of everything.

Use Enumerations

Right now, you have ten bools for race. But only one of those is ever going to be on, and that might prove difficult to ensure long-term. Prefer to instead use an enumeration:

enum Race {
    UNKNOWN,
    DEAD,
    HUMAN,
    ORC,
    GOBLIN,
    ELF,
    LIZARD,
    CAT,
    VAMPIRE,
    WEREWOLF,
    SNK
};


Then we can just have one variable of type Race, that will get set as appropriate - and will definitely only ever refer to one Race. This should also probably be a member variable of Player.

Code Snippets

int Charma = 0;
unsigned int Hunger = 10;
unsigned int Energy = 50;
unsigned int Health = 100;
struct Player {
    int Charma = 0;
    unsigned int Hunger = 10;
    unsigned int Energy = 50;
    unsigned int Health = 100;
};
bool prompt(std::string const& message) {
    std::string answer;
    std::cout << message;
    std::cin >> answer;
    if (answer == "yes") {
        return true;
    }
    else if (answer == "no") {
        return false;
    }
    else {
        throw std::runtime_error("fail"); // or keep looping until
                                          // the user enters something valid
                                          // but definitely do SOMETHING
    }
}
bool get_backpack = prompt("Get the backpack?\n");
enum Race {
    UNKNOWN,
    DEAD,
    HUMAN,
    ORC,
    GOBLIN,
    ELF,
    LIZARD,
    CAT,
    VAMPIRE,
    WEREWOLF,
    SNK
};

Context

StackExchange Code Review Q#106951, answer score: 29

Revisions (0)

No revisions yet.