C ++ Shop Keeper计划_编程黑洞网

我做了一个小小的店主计划，我希望有人批评它，并列出我可以做的所有改进。这是我第一次使用OOP技术，因此我认为有很多地方需要改进。

该程序允许您从商店卖家那里购买（和出售）多达5种商品，并且还具有货币系统。

主要

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

欢迎来到代码审查！在第一个问题上做得很好。

不修改对象状态的成员函数应为const。（例如，简单的get函数应为const）。

#1 楼

以下是一些可以帮助您改进代码的事情。

请确保您已具备所有必需的#include s

Player.h使用std::string但未使用#include <string>。另外，请仔细考虑哪些#include是接口的一部分（并属于.h文件），哪些是实现的一部分。

不要在成员声明中包括类名

Player.h文件包含以下行：

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

它不应包含类名，因此应改为：

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

一般可移植性

如果省略仅Windows包含文件#include "stdafx.h"，则可以将此代码移植。

注意带符号和无符号

在Player::ListInventory例程和其他例程中，代码会将int i与从size_t返回的inventory.size()进行比较，但是size_t是未签名的，而int是签。而是将两个变量都声明为size_t类型。

始终将return设置为适当的值

您的Player:AddItem()例程具有控制路径，该例程导致其结束时没有return输入任何std::string值。这是一个错误，应予以解决。

简化逻辑

代码中的某些地方可以大大简化。例如，当前代码包含以下内容：

//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;
    }
}

可以这样写：

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

在可行的地方使用const

当前的Player::GetName()例程不会（也不应该）修改基础对象，因此应将其声明为const：

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

其他许多不修改底层对象的例程也是如此。

析构函数通常应该是虚拟的

如果我们想从您现有的一个类（例如Player类）中派生一个类，则我们希望将析构函数设为虚拟。请参阅此问题以获取关于原因的更完整说明。类，因此您应该忽略它们，而只是让编译器自动生成它们。

考虑分离输入和输出

现在，许多功能都会发出提示，从用户那里读取内容，然后更新相关对象的内部状态。更具模块化（并且可能更易于维护）的方法会将I / O部分与对象内部状态的更新分开。例如，请参见“模型-视图-控制器”设计模式。

消除“幻数”

代码中有一些数字，例如5和20具有在其特定上下文中具有特定含义。通过使用诸如NUMBER_OF_MENU_ITEMS或OFFERED_PRICE之类的命名常量，该程序将变得更易于阅读和维护。对于常量仅对特定对象有意义的情况，请考虑将该常量作为对象的一部分。

在许多地方使用菜单对象

在您的代码中，您有类似于菜单的内容。您的代码提供了两个选项，然后要求用户根据输入数字选择一个。与其在许多地方重复该代码，不如将其通用。实际上只有提示字符串会发生变化，但是呈现选择和要求输入的基本逻辑都是相同的。

更好地清理用户输入

如果我在菜单提示之一中输入字符串而不是数字，该程序将永远循环。与直接输入int相比，更安全的方法是输入字符串，然后像std::stoi一样将其转换为整数。

忽略return 0

当C ++程序到达main的末尾时，编译器将自动生成代码以返回0，因此没有理由将return 0;明确地放在main的末尾。 />

\ $ \ begingroup \ $
stdafx.h没有特定于Windows的窗口，它只是一个头文件，其中包含最常用的#includes来加快使用预编译头的编译速度。它由GCC，Clang和MSVC支持。 stdafx.h中可能包含一两个特定的窗口，例如tchar.h或targetver.h，但是在跨平台时可以将其删除。
\ $ \ endgroup \ $
– Johnbot
15年12月22日在9:11

\ $ \ begingroup \ $
您能详细说说它不应该包含类名吗？为什么呢
\ $ \ endgroup \ $
–lukas.pukenis
15年12月22日在10:20

\ $ \ begingroup \ $
有两个原因不包含类名。第一个是它根本就没有必要，因为它已经在类声明中了；第二个是出于可读性和一致性，因为您的其他方法都不包含类名。
\ $ \ endgroup \ $
–爱德华
15年12月22日在13:01

\ $ \ begingroup \ $
@Johnbot：更简单的是删除文件。我的Linux机器上没有stdafx.h，是的，我知道它的用途以及MSVC生成它的原因。但是，对于这样的小型项目，编译速度不太可能有太大差异，否则会妨碍可移植性。
\ $ \ endgroup \ $
–爱德华
15年12月24日在2:45

#2 楼

我知道使用注释是个人喜好，但要考虑的是在可能的情况下用描述性变量和函数名称替换它们。

Player.cpp中的示例： />

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

您在这里选择了一个好的描述性函数名称，因此此注释可能不会为一般可读性增加很多价值。

相关阅读：
http://elegantcode.com/2010/04/18/eliminate-comments-the-road-to-clarity/

#3 楼

我相信您的代码中有一个错误：

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

如果将其卖给店主，为什么要增加商品数量？该数字应递减：

--numbOfItems;

实际上，您甚至根本不需要numbOfItems，因为所拥有的项数实质上是inventory向量的大小。这使您不必在购买/出售功能中增加/减少此值，并且还简化了IsInventoryFull()函数的逻辑：

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

InventoryCapacity()错误。当我们谈论某物的容量时，通常是在谈论某物当前可以容纳的最大尺寸。在您的函数中，您要返回的是库存的当前大小，而不是最大大小，即maxNumbItems。可以将其更改为以下内容：

int InventoryCapacity() const
{
    return maxNumbItems;
}

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

此外，由于您正在限制建造中玩家的库存大小，因此我们可以为我们保留该空间提前清点库存：

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

最后，在您的AddItem函数中，不需要按值传递string（因为这会复制它）。节省不必要的开销，并通过const引用传递。而且，不需要分支else。它只是增加了另一个级别的不必要的缩进。您也没有该函数所有分支的有效返回string。这些应该添加：

//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 ""
}

编辑：

您的评论似乎也不一致：

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

当店主让玩家从他们那里购买物品时，他实际上就是在向玩家出售物品。因此，从店主的角度来看，如果玩家正在出售物品，那么他就是在购买物品。相反，如果玩家正在购买物品，则将其出售给玩家。这些令人困惑的评论可能是我突出显示该错误的原因。正如上面提到的，您的函数名称很好，因此我将省略注释。