# C++ Shop Keeper Program

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

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.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;

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:
break;

case 2:
break;

case 3:
break;

case 4:
break;

case 5:
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
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;
}

{
//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";
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()
{
}

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

## 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.

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.

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.

• 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. – Johnbot Dec 22 '15 at 9:11
• Can you elaborate on It should not include the class name ? Why ? – lukas.pukenis Dec 22 '15 at 10:20
• 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. – Edward Dec 22 '15 at 13:01
• @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. – Edward Dec 24 '15 at 2:45

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.

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";
return item;
}

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


EDIT:

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.