Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have made this small text-based RPG in C++ which is based around one quest. I did this to practice what I have learnt so far. How could I improve it? Be as picky as you'd like.

#include <iostream>
#include <stdlib.h>
#include <time.h>

using namespace std;

void riverstead();
void aragornHouse();
void stage1();
void stage2();
void stage3();
void stage4(int &sword, int &gold);
void ratCave();
void attackThief(int &pHealth, int &tHealth);
void thiefDead();
void searchBody();
void questUpdate();
void end();

int input;
int stages[5] = {1, 0, 0, 0, 0};
string qUpdates;
string qStages;

int main() {

    srand (time(NULL));

    system("cls");
    cout << "\n Welcome to RPG!" << endl;
    cout << "\n 1. Play" << endl;
    cout << "\n 2. Exit" << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        qUpdates = "Quest begun";
        qStages = "Talk to Aragorn in his house";
        questUpdate();

        riverstead();

        case 2:
        exit(0);

    }
}

void riverstead() {

    system("cls");
    cout << "\n You are in the town of Riverstead. Where would you like to go?" << endl;
    cout << "\n 1. Aragorn's House" << endl;
    cout << "\n 2. Rat Cave" << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        aragornHouse();

        case 2:
        ratCave();

    }   
}

void aragornHouse() {

    if (stages[0] == 1) {
        system("cls");
        cout << "\n Aragorn: You interested in doing something for me? There's gold in it for you." << endl;
        cout << "\n 1. What do you need?" << endl;
        cout << "\n 2. I'm kind of busy at the moment." << endl;
        cout << "\n> ";
        cin >> input;
        switch (input) {

            case 1:
            stage1();

            case 2:
            system("cls");
            cout << "\n If you find the time, I'll be here." << endl;
            cout << "\n ";
            system("pause");
            riverstead();

        }
    }

    if (stages[1] == 1) {
        stage2();
    }

    if (stages[2] == 1) {
        stage3();
    }

    else {
        stage4(sword, gold);
    }
}

void stage1() {

    system("cls");
    cout << "\n There's a sword that has been in my family for generations and it was recently" << endl;
    cout << "\n passed down to me from my father. " << endl;
    cout << "\n ";
    system("pause");

    system("cls");
    cout << "\n I was walking back from the market the other day and I saw this thief sneaking" << endl;
    cout << "\n out of my house with the sword! " << endl;
    cout << "\n ";
    system("pause");

    system("cls");
    cout << "\n He ran and I followed him to a nearby cave, it's called Rat Cave." << endl;
    cout << "\n ";
    system("pause");

    system("cls");
    cout << "\n I didn't go inside because I had no way to defend myself." << endl;
    cout << "\n 1. I could get that sword for you." << endl;
    cout << "\n 2. I don't think I'm the right person for the job." << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        stages[0] = 0;
        stages[2] = 1;
        system("cls");
        cout << "\n That's great! I'll be waiting right here." << endl;
        cout << "\n ";
        system("pause");

        qUpdates = "Quest updated";
        qStages = "Kill the thief in Rat Cave";
        questUpdate();

        riverstead();

        case 2:
        stages[0] = 0;
        stages[1] = 1;
        system("cls");
        cout << "\n Maybe I could find someone else to do this for me." << endl;
        cout << "\n ";
        system("pause");
        riverstead();

    }
}

void stage2() {

    system("cls");
    cout << "\n Have you changed your mind? Will you retrieve my sword?" << endl;
    cout << "\n 1. Yes, I'm ready to retrieve the sword." << endl;
    cout << "\n 2. I still don't feel like retrieving it." << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        stages[0] = 0;
        stages[1] = 0;
        stages[2] = 1;
        system("cls");
        cout << "\n That's great! I'll be waiting right here." << endl;
        cout << "\n ";
        system("pause");

        qUpdates = "Quest updated";
        qStages = "Kill the thief in Rat Cave";
        questUpdate();

        riverstead();

        case 2:
        system("cls");
        cout << "\n That's a shame. Talk to me if you change your mind." << endl;
        cout << "\n ";
        system("pause");
        riverstead();

    }
}

void stage3() {

    system("cls");
    cout << "\n Have you retrieved the sword yet?" << endl;
    cout << "\n 1. I'm working on it." << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        system("cls");
        cout << "\n Let's hope the thief is still there by the time you get round to doing it." << endl;
        cout << "\n ";
        system("pause");
        riverstead();

    }
}

void stage4(int &gold, int &sword) {

    system("cls");
    cout << "\n Have you retrieved the sword yet?" << endl;
    cout << "\n 1. Yup. Was a piece of cake." << endl;
    cout << "\n 2. Yes, at the price of almost getting killed." << endl;
    cout << "\n> ";
    cin >> input;

    system("cls");
    cout << "\n That's brilliant! I knew you were the right man for the job. Here is the gold," << endl;
    cout << "\n as promised." << endl;
    cout << "\n ";
    system("pause");

    sword = 0;
    system("cls");
    cout << "\n Item removed - Aragorn's Sword" << endl;
    cout << "\n ";
    system("pause");

    gold = gold + 100;
    system("cls");
    cout << "\n Item added - 100 Gold" << endl;
    cout << "\n ";
    system("pause");

    qUpdates = "Quest complete";
    qStages = " ";
    questUpdate();

    end();

}

void ratCave() {

    int pHealth;
    int pDamage;
    int tHealth;
    int tDamage;
    int turn;

    if (stages[0] == 1 || stages[1] == 1) {
        system("cls");
        cout << "\n I haven't really got a good reason to go here." << endl;
        cout << "\n ";
        system("pause");
        riverstead();
    }

    system("cls");
    cout << "\n Thief: I'm warning you, stranger. Leave now!" << endl;
    cout << "\n 1. I've come for my friend's sword." << endl;
    cout << "\n 2. Okay, okay, I'm leaving!" << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        system("cls");
        cout << "\n Ha! You won't be leaving with it!" << endl;
        cout << "\n ";
        system("pause");

        pHealth = rand() % 40 + 80;
        tHealth = rand() % 20 + 40;

        turn = rand() % 2;

        if (turn == 1) {
            system("cls");
            cout << "\n The thief has the first turn." << endl;
            cout << "\n ";
            system("pause");

            tDamage = rand() % 5 + 10;

            pHealth = pHealth - tDamage;
            system("cls");
            cout << "\n The thief attacks you for " << tDamage << " damage!" << endl;
            cout << "\n ";
            system("pause");

        }

        else {
            system("cls");
            cout << "\n You have the first turn." << endl;
            cout << "\n ";
            system("pause");
        }

        attackThief(pHealth, tHealth);

        case 2:
        system("cls");
        cout << "\n That's what I thought." << endl;
        cout << "\n ";
        system("pause");
        riverstead();

    }
}

void attackThief(int &pHealth, int &tHealth) {

    int pDamage;
    int tDamage;

    pDamage = rand() % 10 + 10;
    tDamage = rand() % 5 + 10;

    system("cls");
    cout << "\n Your health: " << pHealth << endl;
    cout << "\n Thief's health:  " << tHealth << endl;
    cout << "\n What would you like to do?" << endl;
    cout << "\n 1. Attack the thief" << endl;
    cout << "\n 2. Attempt to flee" << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        tHealth = tHealth - pDamage;
        system("cls");
        cout << "\n You attack the thief for " << pDamage << " damage!" << endl;
        cout << "\n ";
        system("pause");

        if (tHealth < 1) {
            system("cls");
            cout << "\n You have killed the thief!" << endl;
            cout << "\n ";
            system("pause");
            thiefDead();
        }

        pHealth = pHealth - tDamage;
        system("cls");
        cout << "\n The thief attacks you for " << tDamage << " damage!" << endl;
        cout << "\n ";
        system("pause");

        if (pHealth < 1) {
            system("cls");
            cout << "\n You have been killed by the thief!" << endl;
            cout << "\n ";
            system("pause");
            exit(0);
        }

        attackThief(pHealth, tHealth);

        case 2:
        system("cls");
        cout << "\n Your attempt to flee is unsuccessful." << endl;
        cout << "\n ";
        system("pause");
        attackThief(pHealth, tHealth);

    }
}

void thiefDead() {

    qUpdates = "Quest updated";
    qStages = "Retrieve Aragorn's Sword";   
    questUpdate();

    system("cls");
    cout << "\n What would you like to do?" << endl;
    cout << "\n 1. Search the thief's body" << endl;
    cout << "\n 2. Leave the cave" << endl;
    cout << "\n> ";
    cin >> input;
    switch (input) {

        case 1:
        searchBody();

        case 2:
        riverstead();

    }
}

void searchBody() {

    int gold;
    int sword;

    gold = gold + 20;
    sword = 1;
    stages[2] = 0;
    stages[3] = 1;

    system("cls");
    cout << "\n You found 20 gold and Aragorn's Sword!" << endl;
    cout << "\n ";
    system("pause");

    qUpdates = "Quest updated";
    qStages = "Return to Aragorn";  
    questUpdate();

    return;

}

void questUpdate() {

    system("cls");
    cout << "\n " << qUpdates << " - Aragorn's Sword" << endl;
    cout << "\n ";
    system("pause");

    if (qStages != " ") {
        system("cls");
        cout << "\n " << qStages << endl;
        cout << "\n ";
        system("pause");
    }

    return; 

}

void end() {

    system("cls");
    cout << "\n Thank you for playing RPG! A game made by Elliot Morgan." << endl;
    cout << "\n ";
    system("pause");
    main();

}
share|improve this question
    
    
Sorry, I didn't know that. Thanks for the link. –  Elliot Morgan 7 hours ago
    
are you sure your code work? .. try makes sword and gold variables in aragornHouse() global. –  MORTAL 6 hours ago
    
Yeah, I must've accidentally posted it when it wasn't working. –  Elliot Morgan 6 hours ago
1  
since you are interesting in C++, here link to help you making your code in OOP style. penguinprogrammer.co.uk/rpg-tutorial-2 –  MORTAL 5 hours ago

2 Answers 2

up vote 8 down vote accepted

do not do using namespace std in global scope, instead use it inside the functions or even better only on the things you are using:

int main()
{
  using std::cout;
  ...
}

in your main you have exit(0), instead you should do a return statement.

return EXIT_SUCCESS;

You don't have a default case in your switch statements so if a user enters the wrong number nothing happens and the program ends without notice. It is better to have some kind of error handling. You should also do a function that shows a number of options and lets the user enter a value that is returned if correct that way you save some typing.

e.g.

/**
 * show a number choices and lets the user choose one
 * @returns 1-n
 */
int promptUser( std::vector<std::string>& options );

Your program has the structure of a C program, you should use classes to encapsulate functionality. Identify the objects in the story and create appropriate classes.

e.g. Aragorn, Thief, Player have some common traits


system("pause"); 

calling external programs like that is not a good thing, it opens up a security hole in your application instead use std::getline or similar.


you forgot to initialize some variables e.g.

int gold;   <---
int sword;

gold = gold + 20;

always make it a habit to initialize variables when you declare them.


don't call main() it makes the program flow difficult to follow, instead have a loop in main() if you want to allow restart of the game.

share|improve this answer
    
Thank you for your help. I've already done the first three things and I will move on to the other ones later :) –  Elliot Morgan 7 hours ago

There is a lot to be improved here. Number one is that you have huge blocks of code in super methods that handle entire moves. You should split that up into smaller logical groups for ease of reading, debugging, maintenance, and additions. One example I noticed is that you have this a lot:

cout << "\n 1. Attack the thief" << endl;
cout << "\n 2. Attempt to flee" << endl;

Maybe you should make a method to print the options:

void printOptions(std::vector<std::string> ops)
{
    for (int i = 0; i < ops.size(); i++) {
        std::cout << i + 1 << ". " << ops[i] << std::endl;
    }
}

Then, you can just call this:

printOptions({"Attack the thief", "Attempt to flee"});

This is cleaner than the other version, and is reusable, so it will shorten your code. You can also have as many arguments as you wish, so if you want three options sometime, no problem.

Another problem you have is you are using namespace std;. This is bad, because later you may define your own namespace or use a different one that has some methods named the same, which could cause problems.

Your switch statements should not look like this:

switch (input) {

    case 1:
    searchBody();

    case 2:
    riverstead();

}

Instead, the case statements should be indented, like this:

switch (input) {
    case 1:
        searchBody();

    case 2:
        riverstead();
}

Also, I believe you have an error here. In C++, and most other languages, once a matching case statement is reached, you continue executing all lower case statements. If case 1: is matched and searchBody(); is executed, that means riverstead(); is also executed. You should add a break; statement to the end of each case block:

switch (input) {
    case 1:
        searchBody();
        break;

    case 2:
        riverstead();
        break;        // unnecessary here because it is the last statement, but good practice.
}

I prefer if/else statements to switch statements, but switch statements are sometimes helpful.

Again, you have huge methods that should be split up into smaller blocks. I'm sure there are other things you can clean up, but this will be a good startm

share|improve this answer
    
That's very helpful. Thanks a lot :) –  Elliot Morgan 8 hours ago
1  
Just an observation: To avoid getting compiler warnings, in your printOptions() function, you could change your int i statement to string::size_type i or size_t i, which is what string::size() returns. –  Streppel 5 hours ago
    
@Streppel Good points, but I have never received a compiler warning about this in Visual Studio. –  Hosch250 4 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.