21
\$\begingroup\$

I have made a little shop keeper program and I would love someone to critique it and list all of the improvements I could make. This is my first time using OOP techniques so I imagine there is a lot to be improved upon.

This program allows you to purchase - and sell - up to 5 items from a shop seller and has a currency system too.

Main

// Shop.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include "Player.h"
#include "ShopKeeper.h"

#include <iostream>
#include <algorithm>
#include <vector>
#include <string>

int main()
{
    Player player; //The player
    ShopKeeper shopKeeper; //The shop keeper

    int responce; //Menu navigation
    std::cout << "Greetings " << player.GetName() << ". Feel free to browse my wares." << "\n";
    std::cout << "1: Purchase Items. 2: Sell Items. 3: List Your Items. 4: Show Gold. 5: Exit" << "\n";

    do
    {
        std::cin >> responce;

        switch (responce)
        {
        case 1:
            shopKeeper.PurchaseItem(player);
            break;

        case 2:
            shopKeeper.SellItem(player);
            break;

        case 3:
            player.ListInventory();
            break;

        case 4:
            std::cout << "You have " << player.GetGold() << " gold coins." << "\n";
            break;

        case 5:
            std::cout << "Thank you for shopping." << "\n";
            break;

        default:
            std::cout << "Please enter valid data." << "\n";
            std::cout << "1: Purchase Items. 2: Sell Items. 3: List Your Items. 4: Show Gold. 5: Exit" << "\n";
        }

        std::cout << "1: Purchase Items. 2: Sell Items. 3: List Your Items. 4: Show Gold. 5: Exit" << "\n";

    } while (responce != 5);

    /*
    //This works
    player.AddItem("Mace", 30);
    player.ListInventory();
    std::cout << player.GetGold();
    */

    //Keep window open
    std::string barn;
    std::cin >> barn;

    return 0;
}

ShopKeeper.h

#pragma once
#include "Player.h"

#include <string>

class ShopKeeper
{
private: 



public:
    void PurchaseItem(Player& player); //Shop keeper has player buy items from them
    void SellItem(Player& player); //Shop keeper sells item to player

    ShopKeeper();
    ~ShopKeeper();

};

ShopKeeper.cpp

#include "stdafx.h"
#include "ShopKeeper.h"
#include "Player.h"

#include <iostream>

//Player purchases item from shop keeper
void ShopKeeper::PurchaseItem(Player& player)
{
    //Player player;

    int responce = 0; //Menu navigation
    std::cout << "1: Mace - 30 gold. 2: Bow - 50 gold. 3: Boots - 10 gold. 4: Bearskin - 75 gold. 5: Helmet - 25 gold." << "\n";

    do
    {
        std::cin >> responce;

        switch (responce)
        {
        case 1:
            player.AddItem("Mace", 30);
            break;

        case 2:
            player.AddItem("Bow", 50);
            break;

        case 3:
            player.AddItem("Boots", 10);
            break;

        case 4:
            player.AddItem("Bearskin", 75);
            break;

        case 5:
            player.AddItem("Helmet", 25);
            break;

        default:
            std::cout << "Please enter valid data." << "\n";
            std::cout << "1: Mace - 30 gold. 2: Bow - 50 gold. 3: Boots - 10 gold. 4: Bearskin - 75 gold. 5: Helmet - 25 gold." << "\n";
        }
    } while (responce > 5 || responce < 1);

}

//Shop  keeper sells item to player
void ShopKeeper::SellItem(Player& player)
{
    //Player player;
    int responce = 0;
    player.ListInventory();
    if (responce < player.GetNumbOfItems())
    {
        std::cin >> responce;

        switch (responce)
        {
        case 1:
            player.SellItem(0, 20);
            break;

        case 2:
            player.SellItem(1, 20);
            break;

        case 3:
            player.SellItem(2, 20);
            break;

        case 4:
            player.SellItem(3, 20);
            break;

        case 5:
            player.SellItem(4, 20);
            break;

        default:
            std::cout << "Please enter valid data." << "\n";
            player.ListInventory();
        }
    }

    else
    {
        std::cout << "Item doesn't exist."; 
    }

}

ShopKeeper::ShopKeeper()
{
}


ShopKeeper::~ShopKeeper()
{
}

Player.h

#pragma once

#include <vector>

class Player
{
private:
    const int maxNumbItems = 5; //Maximum number of items that inventory can store

    int goldCoins = 150, //Amount of gold coins the player has
        numbOfItems = 0; //Number of con-current items player holds
    std::vector<std::string> inventory; //Players inventory
    std::string name = "Gorrex"; //Players name

public:
    std::string GetName(); //Get the players name
    std::string AddItem(std::string item, int itemPrice); // Add item to players inventory
    void Player::SellItem(int itemNum, int itemPrice); //Sell item 
    bool IsInventoryFull(); //Check to see if players inventory is full
    int InventoryCapacity(); //Get capacity of inventory
    int GetGold(); //Get players gold
    void ListInventory();
    int GetNumbOfItems();


    Player();
    ~Player();
};

Player.cpp

#include "stdafx.h"
#include "Player.h"

#include <iostream>
#include <ostream>
#include <string>

//Get the players name
std::string Player::GetName()
{
    return name;
}

//Add item to players inventory
std::string Player::AddItem(std::string item, int itemPrice)
{
    //Is players inventory not full?
    if (IsInventoryFull())
    {
        std::cout << "Inventory is full.";
    }

    else
    {
        //Can player afford item?
        if (goldCoins >= itemPrice)
        {
            goldCoins -= itemPrice;
            numbOfItems++;
            std::cout << "You have purchased " << item << "." << "\n";
            inventory.push_back(item); //Add item to inventory
            return item;
        }

        //If player cant afford item 
        else
        {
            std::cout << "You cannot afford this item." << "\n";
        }
    }
}

void Player::SellItem(int itemNum, int itemPrice)
{
    char responce;
    std::cout << "Are you sure you want to sell: " << inventory[itemNum] << "? 'y' - Yes. 'n' - No." << "\n";
    std::cin >> responce;

    switch (responce)
    {
    case 'y':
        numbOfItems++;
        goldCoins += itemPrice;
        inventory.erase(inventory.begin() + itemNum);
        break;

    case 'n':
        std::cout << "That is ok." << "\n"; 
        break;

    default:
        std::cout << "Please enter correct data." << "\n";
    }
}




//Check to see if players inventory is full
bool Player::IsInventoryFull()
{
    //If players inventory isnt full
    if (numbOfItems < maxNumbItems)
    {
        return false;
    }

    //If players inventory is full
    else
    {
        return true;
    }
}

//Return size of players inventory
int Player::InventoryCapacity()
{
    return inventory.size();
}

//Get the players gold
int Player::GetGold()
{
    return goldCoins;
}

//List the players inventory
void Player::ListInventory()
{
    int itemNumb = 0; //item number in menu

    for (int i = 0; i < inventory.size(); i++)
    {
        itemNumb++;
        std::cout << itemNumb << ": " << inventory[i] << "\n";
    }

    /*  //If inventory is empty
    if (inventory.empty())
    {
        std::cout << "inventory is empty" << "\n";
    }*/

}

int Player::GetNumbOfItems()
{
    return numbOfItems;
}

Player::Player()
{
}


Player::~Player()
{
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – SirPython Dec 21 '15 at 21:53
  • \$\begingroup\$ Member functions that don't modify the state of the object should be const. (For example, a simple get-function should be const). \$\endgroup\$ – Juho Dec 21 '15 at 21:58
23
\$\begingroup\$

Here are some things that may help you improve your code.

Make sure you have all required #includes

Player.h uses std::string but doesn't #include <string>. Also, carefully consider which #includes are part of the interface (and belong in the .h file) and which are part of the implementation.

Don't include the class name in member declarations

The Player.h file includes this line:

void Player::SellItem(int itemNum, int itemPrice); //Sell item 

It should not include the class name, so it should instead be written as:

void SellItem(int itemNum, int itemPrice); //Sell item 

General portability

This code could be made portable if you omit the Windows-only include files #include "stdafx.h".

Be careful with signed and unsigned

In the Player::ListInventory routine and various others, the code compares an int i to a size_t as returned from inventory.size(), but size_t is unsigned and int is signed. Instead, declare both variables as size_t types.

Always return an appropriate value

Your Player:AddItem() routine has control paths that cause it to end without returning any std::string value. This is an error and should be fixed.

Simplify your logic

Some places in the code could be greatly simplified. For example the current code includes this:

//Check to see if players inventory is full
bool Player::IsInventoryFull()
{
    //If players inventory isnt full
    if (numbOfItems < maxNumbItems)
    {
            return false;
    }

    //If players inventory is full
    else
    {
            return true;
    }
}

This can be written instead like this:

bool Player::IsInventoryFull()
{
    return numbOfItems >= maxNumbItems;
}

Use const where practical

The current Player::GetName() routine does not (and should not) modify the underlying object, and so it should be declared const:

std::string GetName() const; //Get the players name

The same is true of a number of other routines which don't modify the underlying object.

Destructors should be generally virtual

If we want to derive a class from one of your existing ones, such as the Player class, we will want to have the destructor as virtual. See this question for a fuller explanation as to why.

Don't declare compiler-generated functions

Your constructor and destructor don't do anything in either the Player or ShopKeeper classes, so you should omit them and simply let the compiler generate them automatically.

Consider separating input and output

Right now, many of the functions issue a prompt, reads in the something from the user, and then updates the internal states of the relevant objects. A more modular (and likely more maintainable) approach would separate the I/O portion from the updating of internal state of the objects. For example, see the Model-View-Controller design pattern.

Eliminate "magic numbers"

There are a few numbers in the code, such as 5 and 20 that have a specific meaning in their particular context. By using named constants such as NUMBER_OF_MENU_ITEMS or OFFERED_PRICE, the program becomes easier to read and maintain. For cases in which the constant only has sense with respect to a particular object, consider making that constant part of the object.

Use a menu object

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.

Sanitize user input better

If I enter a string instead of number for one of the menu prompts, the program just loops forever. Rather than inputting an int directly, a safer method is the input a string and then convert it to an integer as with std::stoi.

Omit return 0

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main.

\$\endgroup\$
  • \$\begingroup\$ There is nothing windows specific about stdafx.h, it's just a header file containing the most used #includes to speed up compilation using precompiled headers. It's supported by GCC, Clang and MSVC. There might be one or two windows specific includes in stdafx.h, like tchar.h or targetver.h but they can be removed when making it cross platform. \$\endgroup\$ – Johnbot Dec 22 '15 at 9:11
  • \$\begingroup\$ Can you elaborate on It should not include the class name ? Why ? \$\endgroup\$ – lukas.pukenis Dec 22 '15 at 10:20
  • \$\begingroup\$ There are two reasons not to include the class name. The first is that it's simply not necessary since it's already inside the class declaration, and the second is for readability and consistency since none of your other methods include the class name. \$\endgroup\$ – Edward Dec 22 '15 at 13:01
  • \$\begingroup\$ @Johnbot: simpler still is removing the file. There is no stdafx.h on my Linux box, and yes, I know what it's for and why MSVC generates it. However, for a small project like this it's unlikely to make much difference to compile speed and is otherwise an impediment to portability. \$\endgroup\$ – Edward Dec 24 '15 at 2:45
6
\$\begingroup\$

I recognise that the use of comments is a personal preference, but something to think about is replacing them where possible with descriptive variable and function names.

An example from Player.cpp:

//Check to see if players inventory is full
bool Player::IsInventoryFull();

You've chosen a good descriptive function name here, so this comment may not add a lot of value to general readability.

Relevant reading: http://elegantcode.com/2010/04/18/eliminating-comments-the-road-to-clarity/

\$\endgroup\$
1
\$\begingroup\$

I believe you have a bug in your code here:

void Player::SellItem(int itemNum, int itemPrice)
{
    char responce;
    std::cout << "Are you sure you want to sell: " << inventory[itemNum] << "? 'y' - Yes. 'n' - No." << "\n";
    std::cin >> responce;

    switch (responce)
    {
    case 'y':
        numbOfItems++; // <-- bug here
        goldCoins += itemPrice;
        inventory.erase(inventory.begin() + itemNum);
        break;

    case 'n':
        std::cout << "That is ok." << "\n"; 
        break;

    default:
        std::cout << "Please enter correct data." << "\n";
    }
}

Why are you increasing the number of items you have if you're selling it back to the shopkeeper? This number should be decremented:

--numbOfItems;

Actually, you don't even need numbOfItems at all since the number of items you have is essentially the size of your inventory vector. This frees you from having to increment/decrement this value in the buy/sell functions and it also simplifies the logic for your IsInventoryFull() function:

bool IsInventoryFull() const
{
    return inventory.size() < maxNumbItems;
}

You have the logic for InventoryCapacity()incorrect. When we speak about the capacity of something, we're generally talking about the maximum size something can currently hold. In your function, you're returning the current size of your inventory which is not its maximum size - that's maxNumbItems. This can be changed to be the following:

int InventoryCapacity() const
{
    return maxNumbItems;
}

int getNumbOfItems() const
{
    return inventory.size();
}

Also, since you're limiting the size of the inventory for the player at construction, we can reserve that space forour inventory ahead of time:

Player()
{
    inventory.reserve(maxNumbOfItems);
}

Finally, in your AddItem function, there's no need to pass the string by value (as this copies it). Save yourself the unneeded overhead and pass in by const reference. Moreover, there's no need for the branching else. It just adds another level of unnecessary indentation. You also don't have a valid return string for all branches of the function. These should be added:

//Add item to players inventory
std::string Player::AddItem(const std::string &item, int itemPrice)
{
    //Is players inventory not full?
    if (IsInventoryFull())
    {
        std::cout << "Inventory is full.";
        return "";
    }

   //Can player afford item?
    if (goldCoins >= itemPrice)
    {
        goldCoins -= itemPrice;
        numbOfItems++;
        std::cout << "You have purchased " << item << "." << "\n";
        inventory.push_back(item); //Add item to inventory
        return item;
    }

    //If player cant afford item 
    std::cout << "You cannot afford this item." << "\n";
    return ""
}

EDIT:

It looks like your comments are inconsistent as well:

void PurchaseItem(Player& player); //Shop keeper has player buy items from them
void SellItem(Player& player); //Shop keeper sells item to player

When the shopkeeper has the player buy items from them, he is effectively selling the player the items. So from the shopkeeper's point of view, if the player is selling items, he is buying them; conversely, if the player is buying items, he is selling them to the player. These confusing comments are probably the reason for the bug I highlighted. As someone mentioned above, you have good function names so I would just omit the comments.

\$\endgroup\$

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.