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

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

    int responce; //Menu navigation
    std::cout > responce;

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

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

        case 3:
            player.ListInventory();
            break;

        case 4:
            std::cout > barn;

    return 0;
}

ShopKeeper.h

#pragma once
#include "Player.h"

#include 

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

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

int responce = 0; //Menu navigation
std::cout > 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);

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

StackExchange Code Review Q#114692, answer score: 23