-1
\$\begingroup\$

I have been messing around with practicing some making a little console game, and I would like to receive some advice and tips on how to improve my code.

Some of the code is not done, meaning that some of the functions are not done and some are not called but I have tested it and it does all works.

Some of my concerns are:

  • I don't know if I used dynamic memory allocation properly.
  • I am not sure if I should be using pointers in this.
  • I am not sure if I am following all the "standards" I should be.

Player.h

#ifndef PLAYER_H
#define PLAYER_H

#include <string>
#include <vector>
using namespace std;

class Player
{
public:
    void DisplayPlayerInv(vector<string>Inv);  //Gets the players inv
    //void AddPlayerInv(vector<string>Inv);
    void PlyrArmor(int Armor); //Players armor

    string PlyrName(string name); //Gets and displays the players name 
    vector<string>Inv;
private:
    vector<string*>pInv; // vector of strings to store items in inventory
    vector<string>::const_iterator it; // iteraotr for inv

    int health;
    int gold;
    int Armor;

    string name;
};
#endif

Player.cpp

#include <iostream>
#include <string>
#include <vector>
#include "Player.h"
using namespace std;


void Player::DisplayPlayerInv(vector<string>Inv) // goes through the players inventory
{
    for (it = Inv.begin(); it < Inv.end(); ++it)
    {
        cout << *it << endl;
    }
}

void Player::PlyrArmor(int Armor) // gets the players armor
{
    //Add stuff here later
}

string Player::PlyrName(string name) // players name
{
    return name;
}

store.h

#ifndef SHOPS_H
#define SHOPS_H
#include <iostream>
#include <string>
#include "Player.h"
using namespace std;

class BaseStore: public Player
{
public:
    virtual void SellItems() = 0;  // function to sell items
    virtual void BuyItems() = 0; // function to buy items
    virtual int SMenu(int choice, char LChoice, bool InStore) = 0; // is the main menu for the store

    //virtual string ListItems() = 0; // lists the items in the store

protected:
    int choice; // used for choices inside the store
    char LChoice; // used for Y or N inside the store
    static bool InStore;
};

class Store1 : public BaseStore
{
public:
    virtual void SellItems(); // function to sell items
    virtual int SMenu(int choice, char LChoice, bool InStore); // used for store menu
    virtual void BuyItems(); // function to buy items

    //virtual string LiteItems() = 0; // lists the items inside the store
};

#endif

Store.cpp

#include <iostream>
#include "Store.h"
#include "Player.h"
#include <string>
#include <vector>
using namespace std;

void Store1::SellItems() // sell items menu
{
    &Player::DisplayPlayerInv;
}

void Store1::BuyItems() // buy items menu
{

}

int Store1::SMenu(int choice, char LChoice, bool InStore)  // stores menu
{
    InStore = true;
    while (InStore == true)
    {
        cout << "Hello welcome to the store what would you like to do?\n\n";
        cout << "1. Buy Items\n";
        cout << "2. Sell Items\n";
        cout << "3. leave\n";
        cin >> choice;

        if (choice == 1 && LChoice != 'N' && LChoice != 'n')
        {
            cout << "would you like to buy items? (Y/N)\n";
            cin >> LChoice;
            BuyItems();
        }
        else if (choice == 2 && LChoice != 'N' && LChoice != 'n')
        {
            cout << "Would you like sell items? (Y/N)\n";
            cin >> LChoice;
            SellItems();
        }
        else if (choice == 3 && LChoice != 'N' && LChoice != 'n')
        {
            cout << "Would you like to leave the store? (Y/N)\n";
            cin >> LChoice;
            return 0;
            InStore = false;
        }
        else { cout << "You Have Made a invalid Choice"; }
    }

    return 0;
}

Enemey.h

#ifndef ENEMEY_H
#define ENEMEY_H
#include <string>
using namespace std;

class Enemey
{
public:
    virtual int BasicEnemeyStats(int health, int gold, int Attack, int Armor) = 0; // pure virtual function for the enemey stats
    virtual string EnemeyName(string name) = 0; // gets the enemey name
protected:
    int health;
    int Attack;
    int gold;
    int Armor;
    string name;
};

//Enemey classes
class Theif : public Enemey
{
public:
    virtual int BasicEnemeyStats(int health, int gold, int Attack, int Armor);
    virtual string EnemeyName(string name);
protected:
    int Attack;
    int Armor;
};

class Troll : public Enemey
{
public:
    virtual int BasicEnemeyStats(int health, int gold, int Attack, int Armor); // Enemey Stats
    virtual string EnemeyName(string name); // gets the Enemeys Name
protected:
    int Attack;
    int Armor;
};

//Boss Classes
class Boss : public Enemey
{
public:
    virtual int BasicEnemeyStats(int health, int gold, int Attack, int Armor); // Enemey stats
    int DamageMulti(int Attack); // Multiplys damage based on the players armor or attack
    virtual string EnemeyName(string name); // enemey's name
protected:
    int Armor;
    int Attack;
};

#endif

Enemey.cpp

#include <iostream>
#include "Enemey.h"
#include <string>
using namespace std;

// Boss Functions
int Boss::BasicEnemeyStats(int health, int gold, int Attack, int Armor) // Bosses stats 
{
    health = 100; // place holder for now 
    gold = 0;
    Armor = 50;
    Attack = 50;

    return (health, gold, Armor, Attack);
}

int Boss::DamageMulti(int Attack) // damage multiplyer
{
    int DamageMulti = Attack * 2;   //(PlyrArmor) this is what needs to be changed to divide by 2 from player armor stat

    return DamageMulti;
}

string Boss::EnemeyName(string name) // gets the enemey name
{
    return name;
}

//Enemey Functions

string Theif::EnemeyName(string name)
{
    return name;
}

int Theif::BasicEnemeyStats(int health, int gold, int Attack, int Armor) 
{
    health = 100;
    gold = 0;
    Attack = 20;
    Armor = 5;

    return (health, Attack, Armor);
}

string Troll::EnemeyName(string name)
{
    return name;
}

int Troll::BasicEnemeyStats(int health, int gold, int Attack, int Armor)
{
    health = 100;
    gold = 0;
    Attack = 30;
    Armor = 12;

    return (health, Attack, Armor);
}

World.h

#ifndef WORLD_H
#define WORLD_H
#include <iostream>
#include <string>
#include <vector>
using namespace std;

class World // class for the base game basicly the Parent of every other class in the game
{
public:
    void Story(int SChoice, bool NameSelectDone); // is for the story int SChoice stands for Story Choice
    void SetChoice(char LChoice); //  Sets SChocie
    bool NameSelect(bool NameSelectDone, string* plyrName); // Name Set function
    void SaveGame(); // save game functions
    void GameOver(bool IsGameOver, bool NameSelectDone); // Game over function
    int QuitGame(); // quit game function
    void DisplayControls();


private:
    int SChoice;
    char LChoice;
    string* plyrName;
    bool NameSelectDone;
    bool PlayAgain;
    bool IsGameOver;
    bool PlayerAlive;

};
#endif

World.cpp

#include <iostream>
#include "World.h"
#include "Enemey.h"
#include "Player.h"
#include "Store.h"
#include <string>
#include <fstream>
#include "stdlib.h"
using namespace std;

void World::Story(int SChoice, bool NameSelectDone)// Is the story for the game                              
{
    Player* p1 = new Player;// player obj
    Store1* s1 = new Store1; // store obj
    Theif* t1 = new Theif;

    bool PlayerAlive = true; // sees if the player is alive
    NameSelectDone = true; // Name selection variable
    IsGameOver = false; // Game is not over by default
    LChoice;// Yes or no character


    while (NameSelectDone == true && IsGameOver == false && LChoice != 'Q' && LChoice != 'q')
    {
        cout << "Before we begin at anytime if you type 'S' you can save the game, also at anytime if you type 'Q' you can quit the game.\n";
        cout << "Also your choices will afect the outcome of the story so pick wisely\n";
        cout << "If you use the Letter 'C' you can display all the controls.\n";
        cout << "Type any letter to continue....\n";
        cin >> LChoice;

        system("cls");

        cout << "While travling with a group of warriors from the kingdom of 'PlaceHolder'\n";
        cout << "It began to storm as you hear a schreech so loud that it could shatter glass.\n";
        cout << "You look into the stormy clouds and see the sky has been torn and evil forces from the darkness start to pour into the land.\n";
        cout << "Creatures so dark that men begin to reek of piss and other things.\n You look down and see that your land and everyone you have ever known or loved is being slaughterd\n\n";
        cout << "And burned to the ground.\n";
        cout << "While in shock you hear loud stomps and see a wall of darkness coming towards you. the warriors you are with are turned to ash with out any effort.\n";
        cout << "The wall of darkness approches you and then everything fades to black.........................\n\n";
        cout << endl;
        cout << "Type any letter to continue....\n";
        cin >> LChoice;

        system("cls"); // clears the screen

        cout << "You wake up after being blacked out, you proceed to stand up and look into the land and see that forests, villages, and farms are burned to the ground.\n";
        cout << "while starring in disbelif you hear a lound shreek you have two paths.\n";
        cout << "1. Head up the mountains.\n";
        cout << "2. Head down the path towards the town.\n";
        cin >> SChoice; // select choice used for numbers

        if (SChoice == 1)
        {
            cout << "Type any letter to continue....\n";
            system("cls");

            cout << "You Choose to head to the mountains....\n";
            cout << "You begin to head to the mountains, on your way a bear spots you in a cave.\n";
            cout << "But something is diffrent...\n";
            cout << "This bear appears to be made of darkness, pure evil.\n" << "The bear begins to charge you, you have two choices.\n\n";
            cout << "1. Fight the bear\n";
            cout << "2. Run for your life\n";
            cin >> SChoice;

            switch (SChoice)
            {
            case 1:
            {
                system("cls");
                cout << "The bear proccedes to charge you, you draw your fists quickly....\n";
                cout << "Type any letter to continue....\n";
                cin >> LChoice;
                cout << "The bear smashes into you proceding to rip you to shreds.\n";
                cout << "Now with your legs ripped of and your guts and intestines spilling from your torso, you vision fades to black and you die....\n";

                IsGameOver = true;
                system("cls");
                GameOver(NameSelectDone, IsGameOver);
                break;
            }
            case 2:
            {
                system("cls");
                cout << "You choose to run for you life.\n";
                cout << "While running you trip over a rock and die.\n";

                IsGameOver = true;
                system("cls");
                GameOver(NameSelectDone, IsGameOver);
                break;
            }
            }

        }
        else if (SChoice == 2)
        {
            cout << "You start walking down the path and you see dead bodies\n.";
            cout << "There is gear from a soilder on the ground what you like to do with it?\n";
            cout << "1. Pick It Up\n";
            cout << "2. keep walking\n";
            cin >> SChoice;

            if (SChoice == 1)
            {
                cout << "\n\n(Just a quick note if you wish to show what is in your inv type 'I'\n";
                cout << "Basic Armor Equiped\n";
                p1->Inv.push_back("Basic Armor");
                cout << "Basic Sword Equiped\n";
                p1->Inv.push_back("Basic Sword");
            }

            else if (SChoice == 2)
            {
                system("cls");
                cout << "you keep walking and die...\n";
            }

            system("cls");
            cout << "I have no idea on what to add so i will just call a bunch of things for practaice.\n\n";
            cout << "Type a letter to continue...\n";
            cin >> LChoice;
            system("cls");

            cout << "What would you like to do?\n";
            cout << "1. Store";
            cout << "2. call enemeys(stats not working)\n";
            cout << "3. End game\n\n";
            cin >> SChoice;

            switch (SChoice)
            {
            case 1:
            {
                bool InStore = true;
                s1->SMenu(SChoice, LChoice, InStore);
                break;
            }
            case 2:
            {
                t1->EnemeyName("Boogie");
                t1->BasicEnemeyStats(100, 0, 10, 10);
                break;
            }
            case 3:
            {
                QuitGame();
                break;
            }
            }



        }

        if (LChoice == 'Q' || LChoice == 'q') // Quit game
        {
            QuitGame();
            NameSelectDone = false;
        }
        if (LChoice == 'S' || LChoice == 's') // Save game
        {
            SaveGame();
        }

        if (LChoice == 'I' || LChoice == 'i' || SChoice == 'I' || SChoice == 'i')
        {
            p1->DisplayPlayerInv(p1->Inv);
        }

        if (LChoice == 'C' || LChoice == 'c' || SChoice == 'C' || SChoice == 'c')
        {
            DisplayControls();
        }

        delete p1;
        delete s1;
        delete t1;
    }
}

void World::SetChoice(char LChoice) // sets the players choice
{

}

bool World::NameSelect(bool NameSelectDone, string* plyrName) // Player name select loops untill user enters Y/N
{

    Player p1;
    char LChoice;

    while (NameSelectDone == false)
    {
        cout << "Hello welcome to the world of PlaceHolder\n";
        cout << "please enter in your name:";
        cin >> *plyrName;
        cout << "\nIs: " << p1.PlyrName(*plyrName) << " Your name? (Y/N)";
        cin >> LChoice;
        if (LChoice == 'N' || LChoice == 'n')
        {
            NameSelectDone = false;
        }
        else { NameSelectDone = true; }
    }

    return (NameSelectDone);
}

void World::SaveGame() // Save the game
{
    cout << "Saving the game...\n";
    //Add save function
    cout << "Game has been saved\n";
}

int World::QuitGame() // Quit and saves the game aswell
{
    cout << "If you havent saved the game i will save it for you";
    SaveGame();
    cout << "Thank you for playing";
    return 0;
}

void World::GameOver(bool IsGameOver, bool NameSelectDone) // Game over function
{
    cout << "You have died would you like to play again ? (Y/N)\n";
    cin >> LChoice;
    if (LChoice == 'Y' || LChoice == 'y')
    {
        IsGameOver = false;
        NameSelectDone = true;
        cout << "Game is ready to restart...\n" << " (Type any letter to continue)\n";
        cin >> LChoice;
        system("cls");
        Story(NameSelectDone, SChoice);
    }
    else if (LChoice == 'N' || LChoice == 'n')
    {
        cout << "Quitting game...\n";
        QuitGame();
    }
}

void World::DisplayControls()
{

    cout << "At any time you can type these letters to do certain things.\n\n";
    cout << "Q or q exits the game\n";
    cout << "S or s Saves the game\n";
    cout << "I or i Shows what you have in your inventory\n";
    cout << "C or c Shows the controls menu\n";
    cout << "Type any letter to continue...\n";
    cin >> LChoice;
    system("cls");
}
\$\endgroup\$
  • \$\begingroup\$ The code does work when i say that it is not finished i mean the functions that are not called upon or are empty \$\endgroup\$ – Exxor Aug 8 '16 at 18:23
  • \$\begingroup\$ In other words, you're presenting us your menu navigation and overall structure, and asking us to ignore the not-yet-implemented player/enemy stats. I've edited your post into shape, it's probably reviewable, but IMO you would be better off either leaving out the unimplemented "future features" code, or waiting to implement it before you put it up for review. \$\endgroup\$ – Mathieu Guindon Aug 8 '16 at 20:32
1
\$\begingroup\$

code level comments

nice looking code, well structured, some good classes, some comments +++, good names

dont do using namespace std. It pollutes your namespace too much. Just get used to saying std::string etc.

dont repeat part of you naming when already in a named scope.

class Player
{
public:
    void DisplayPlayerInv(vector<string>Inv);  //Gets the players inv
    //void AddPlayerInv(vector<string>Inv);
    void PlyrArmor(int Armor); //Players armor

    string PlyrName(string name); //Gets and displays the players name 

should be

class Player
{
public:
    void DisplayInv(vector<string>Inv);  //Gets the players inv
    //void AddInv(vector<string>Inv);
    void Armor(int Armor); //Players armor

    string Name(string name); //Gets and displays the players name 

adopt a member variable naming standard. I have been shot down for this one, but I still think most people think its valuable. I use m_, the other common one is xxx_. Thus Player becomes:

private:

int health_;
int gold_;
int Armor_;

string name_;

};

that vector of pointers to strings looks very suspicious

vector<string*>pInv;

I am sure you dont need it, just use vector<string> (or std::vector<std::string>), string will do the right thing

Having an iterator as a member variable is also very odd. They are normally transient things in functions.

vector<string>::const_iterator it; // iteraotr for inv

In general dont put explicit IO directly in the classes. Ie

void Player::DisplayPlayerInv(vector<string>Inv) // goes through the players inventory
{
    for (it = Inv.begin(); it < Inv.end(); ++it)
    {
        cout << *it << endl;
    }
}

if you decide you want to display the output a different way you are stuck. Better to pass in a stream to write to or a function to invoke. I would (in this case) simply return the inventory as a read only list so that I can do what I want with it.

be consistenat with names. It makes a huge difference with 10 times as much code, get into good habits

  int health;
    int gold;
    int Armor;

why is Armor upper case?. Typically upper case names mean functions or properties

This is odd

   string Player::PlyrName(string name) // players name
    {
        return name;
    }

I assume its because you intend(ed?) to have 2 functions one to read the name (with no args) and one to set it (with one arg) , this is a hybrid of the 2

Next one look odd too. class BaseStore: public Player {

A Store (place where I can buy and sell stuff) is a type of Player? I dont think so. A Player goes to a store to buy and sell things, or at least thats what I expect to see. This smells odd

\$\endgroup\$
  • \$\begingroup\$ Thank you for the advice on the part to display the inventory could you include some code so i could get an idea on how to improve it. Other than that it was very helpful thank you. Also could i use using std::endl ect. \$\endgroup\$ – Exxor Aug 8 '16 at 23:57
1
\$\begingroup\$

Character Design

Right now, your Player and Enemy classes share quite a few of the same characteristics (e.g., health, gold, armor and name).

The most obvious (but not necessarily preferable) way to eliminate this duplication would be to define a base class (e.g., Character) to hold the characteristics common to all characters. Then your Player and Enemy would inherit from that, and add their own little bits and pieces.

Another possibility would be to use template parameters to create characters. This tends to be a little more advanced and can be a little more difficult to do well, but can also improve performance and simplify some of the code. It's not immediately clear whether the benefit would justify the cost in your case or not.

Comparisons

You have (for example) code like if (NameSelectDone == true && IsGameOver == false [...].

At least in my opinion, it would be better to eliminate the comparisons, so this became: if (NameSelectDone && ! IsGameOver) (or you can use not IsGameOver).

At least in my opinion, this makes the code substantially more readable.

What to make a class

Right now, a player has a single int to represent their armor. Depending on how complex of rules you might want to implement, it may well make sense for armor to be a class of its own, complete with (for example) characteristics about its resistance to various kinds of attacks (e.g., a wooden shield might be quite strong vs. a sword attack, but much weaker vs. a fire attack).

system

There are times that it makes sense to use system, but at least in my opinion, clearing the screen isn't one of them. Since this only works on Windows anyway, I'd write some code for the Win32 API to do the job, such as something on this order:

#include <windows.h>

void clear_screen(char fill = ' ') { 
    COORD tl = {0,0};
    CONSOLE_SCREEN_BUFFER_INFO s;
    HANDLE console = GetStdHandle(STD_OUTPUT_HANDLE);   
    GetConsoleScreenBufferInfo(console, &s);
    DWORD written, cells = s.dwSize.X * s.dwSize.Y;
    FillConsoleOutputCharacter(console, fill, cells, tl, &written);
    FillConsoleOutputAttribute(console, s.wAttributes, cells, tl, &written);
    SetConsoleCursorPosition(console, tl);
}

Embedded text

Right now, Story consists primarily of writing various chunks of text to the screen. Rather than embedding all that text directly into the code, I'd at least consider putting it into an external file (or multiple files). In the long term, the makes it quite a bit easier if (when?) you get to the point that you need/want to do things like translate to another language, or even just rewrite the story a bit.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feed back it does help me out a lot i will try to do the improvements that you suggested \$\endgroup\$ – Exxor Aug 8 '16 at 23:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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