2
\$\begingroup\$

I'm a beginner programmer and I was hoping someone would be able to review my program. Everything has been tested for user validation, error-checking and conforms to the specification with a few slight additions of my own. I want to know what I've done well, what I haven't done well, improvements I can make e.g. how can I can make code structure more efficient. Are there any parts of my program which can be simplified i.e. have I over-thought / over-complicated any parts of my program and if I have is there an easier way? I'll be absolutely grateful to anyone who does! There are five parts to my program, a struct "specification" and "implementation" for both "Item" (singular) and "Items" (plural), and the "Main.cpp". I've based my program on the following scenario...

A system that allows one to enter the items that they are selling, the quantity sold, the price of each of the items. They would like the program to be able to create suggestions based on the data provided.

Input:

Item name

Sale price

Quantity sold

Output:

Quantity of items sold

Total price of items sold

Most sold item

Least sold item

The difference in sales between the most and least sold items

Estimated operational cost (50% of sales)

Tax paid (20% of sales)

Profit (Total after tax and operational costs)

If any number of products constitutes less than 5% of sales individually, a warning should be displayed

Your program will also need to work within the following constraint:

There must be at least 5 products that can be entered

Sales System.cpp : This file contains the 'main' function. Program execution begins and ends there.

#include <iostream>
#include <vector>
#include <string>
#include <iomanip>
#include "Item.hpp"
#include "Items.hpp"

//When called this asks the user how many items they would like to add and the item number has to be greater than 5.
//Through the parameter is passed a vector of items.
//The ContinueOptions() procedure is there to allow people to decide when they want to go back to the menu.
void ContinueOptions()
{
    bool bValid = false;
    char cInputCommandPrompt = 0;
    do{
        std::cout << "Press ""y"" to continue: ";
        bValid = bool( std::cin >> cInputCommandPrompt );

        if (bValid)
        {
            cInputCommandPrompt = std::toupper(static_cast<unsigned char>(cInputCommandPrompt));
            bValid = cInputCommandPrompt == 'Y';
        }

        if (!bValid)
        {
            std::cin.clear();
            std::cin.ignore(100, '\n');
            std::cout << "Please try again.\n";
        }
    } while(!bValid );
    std::cout << "\n";
}


void DisplayMenu()
{
    std::cout << "What would you like to do?\nChoose a number from the Menu...\n";
    std::cout << "\t1. Add another item\n";
    std::cout << "\t2. Display items\n";
    std::cout << "\t3. Total quantity of items sold\n";
    std::cout << "\t4. Total price of items sold\n";
    std::cout << "\t5. Most sold item\n";
    std::cout << "\t6. Least sold item\n";
    std::cout << "\t7. Difference in sales between the most and least sold items\n";
    std::cout << "\t8. Estimated operational cost (50% of sales)\n";
    std::cout << "\t9. Show Tax paid (20% of sales)\n";
    std::cout << "\t10.Profit (Total after tax and operational costs)\n";
    std::cout << "\t11. Show additional options\n";
    std::cout << "\t0. Quit.\n";
    std::cout << "\n";
}

void DisplayAdditionalMenu()
{
    std::cout << "What would you like to do?\nChoose a number from the Additional Menu...\n";
    std::cout << "\t1. Delete an item\n";
    std::cout << "\t2. Search for specific item\n";
    std::cout << "\t0. Back to main menu\n";
    std::cout << "\n";
}

void AdditionalMenu(Items &Items)
{
    bool bExitAdditionalMenu = false;
    do
    {
        DisplayAdditionalMenu();
        int iAdditionalMenuSelection = 0;
        std::cout << "Enter a number from the additional menu: ";
        std::cin >> iAdditionalMenuSelection;
        if(std::cin.good())
        {
            switch(iAdditionalMenuSelection)
            {
                case 1:
                    Items.DeleteAnItem();
                    ContinueOptions();
                    break;
                case 0:
                    bExitAdditionalMenu = true;
                default:
                    std::cout << "Invalid input. Try again!\n";
                    continue;
            }

        }
        else
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
            std::cout << "Sorry, invalid input input. Try again!";
            std::cout << "\n";
            ContinueOptions();
        }

    }while(bExitAdditionalMenu == false);

}
int main()
{
    Items Items;
    bool bExitProgram = false;
    int iMenuSelection = 0;
    int iIndexOfMostSoldItem = 0;
    int iIndexOfLeastSoldItem = 0;
    //If this variable is greater than 1 (which will increment within the do / while) then a continue option will display.
    int iNumber = 1;

    std::cout << "===================================\n";
    std::cout << "\t\tSales System\n";
    std::cout << "===================================\n";
    Items.GetItemInformation();
    Items.DisplayWarning();
    do{
        iNumber++;
        if (iNumber > 1)
        {
            ContinueOptions();
        }
    DisplayMenu();
    std::cout << "Enter a number from the menu: ";
        std::cin >> iMenuSelection;
    if(std::cin.good())
    {
        std::cout << "\n";
        switch (iMenuSelection)
        {
            case 1:
                std::cout << "You've chosen option 1.\n";
                std::cin.ignore();
                Items.AddNewItem();
                break;
            case 2:
                std::cout << "You've chosen option 2.\n";
                Items.PrintItems();
                break;
            case 3:
                std::cout << "You've chosen option 3.\n";
                std::cout << "The total quantity sold is: " << Items.CalculateQuantitySold() << ".\n";
                break;
            case 4:
                std::cout << "You've chosen option 4.\n";
                std::cout << "Total Amount of Items Sold: £" << Items.CalculateTotalSales() << "\n";
                break;
            case 5:
                std::cout << "You've chosen option 5.\n";
                std::cout << "Displaying most sold item...\n";
                iIndexOfMostSoldItem = Items.MostSoldItem();
                Items.DisplayVectorOfItemsAtIndex(iIndexOfMostSoldItem);
                break;
            case 6:
                std::cout << "You've chosen option 6.\n";
                std::cout << "Displaying least sold item...\n";
                iIndexOfLeastSoldItem = Items.LeastSoldItem();
                Items.DisplayVectorOfItemsAtIndex(iIndexOfLeastSoldItem);
                break;
            case 7:
                std::cout << "You've chosen option 7.\n";
                std::cout << "The difference between the most and least is by: ";
                std::cout << Items.CalculateDifferenceBetweenLeastAndMostSold() << ".\n";
                break;
            case 8:
                std::cout << "You've chosen option 8.\n";
                std::cout << "Estimated operational cost (50% of sales): ";
                std::cout << Items.CalculateEstimatedOperationalCost() << ".\n";
                break;
            case 9:
                std::cout << "You've chosen option 9.\n";
                std::cout << "Tax paid (20% of sales): ";
                std::cout << Items.CalculateTaxPaid() << ".\n";
                break;
            case 10:
                std::cout << "You've chosen option 10.\n";
                std::cout << "Profit (Total after tax and operational costs): ";
                std::cout << Items.CalculateProfit() << ".\n";
            case 11:
                AdditionalMenu(Items);
                break;
            case 0:
                std::cout << "You have exited the program.\n";
                bExitProgram = true;
                break;
            default:
                std::cout << "Invalid command prompt. Try again!\n";
                continue;
        }
    }
    //This will essentially clear the invalid characters the user entered and will prompt them to try again by re-looping back to the start.
    else
    {
        std::cin.clear();
        std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
        std::cout << "Sorry, invalid input input. Try again!";
        std::cout << "\n";
        ContinueOptions();
    }

    }while(bExitProgram == false);
    std::cout << "\n";
}

Item.hpp Sale System

#ifndef Item_hpp
#define Item_hpp
#include <iostream>
#include <vector>
#include <string>
#include <iomanip>
#include <stdio.h>
//This struct models as a basic item which includes its name, price and quantity sold.
struct Item{
private:
    std::string sItemName;
    double dSalePrice;
    int iQuantitySold;
public:
    Item(std::string s, double d, int i);
    std::string GetItemName()const;
    double GetItemSalePrice()const;
    int GetItemQuantity()const;

    double GetItemSale();
};
#endif /* Item_hpp */

Item.cpp Sale System

#include "Item.hpp"
#include <stdio.h>
#include <iostream>
#include <vector>
#include <string>
#include <iomanip>

Item::Item(std::string s, double d, int i):sItemName(s),dSalePrice(d),iQuantitySold(i){
}
std::string Item::GetItemName()const{return sItemName;}
double Item::GetItemSalePrice()const{return dSalePrice;}
int Item::GetItemQuantity()const{return iQuantitySold;}

double Item::GetItemSale()
{
    return dSalePrice * iQuantitySold;
}

Items.hpp Sale System This is the specification of the "Items" (plural) struct

#ifndef Items_hpp
#define Items_hpp

#include <stdio.h>
#include <iostream>
#include <vector>
#include <string>
#include <iomanip>
#include "Item.hpp"

struct Items{
    //The vector<Items>ItemsVec is private so only member functions/procedures have access to it.
private:
    std::vector<Item>ItemsVec;
public:
    //The ContinueOptions() procedure is there to allow people to decide when they want to go back to the menu.
    //Memeber functions/procedures of Items prototypes ()
    bool CheckIfItemExists(std::string sInputName);
    void AddNewItem(); //PROTOTYPE of procedure that adds a new item to the vecto
    void GetItemInformation();//PROTOTYPE that allows user to enter in the amount of items (and number of how many will be the amount of times the for loop iterates to add desired number of items through means of push_back)
    int CalculateQuantitySold();//PROTOTYPE of function that calculates the total quantity sold for all items
    int MostSoldItem(); //PROTOTYPE of function that calculates most item using a For Loop.
    int LeastSoldItem(); //PROTOTYPE of function that calculates least item using a For Loop.
    void DisplayVectorOfItemsAtIndex(int iIndex); //PROTOTYPE that expects iIndex as a parameter to ascertain which index to display in vector.
    double CalculateDifferenceBetweenLeastAndMostSold();//PROTOTYPE for calculating difference between the least amount sold and the highest.
    double CalculateTotalSales(); //PROTOTYPE for calculating the total sales
    double CalculateEstimatedOperationalCost(); //PROTOTYPE for calculating the estimated operational cost
    double CalculateTaxPaid(); //PROTOTYPE for calculating amount of tax paid.
    double CalculateProfit(); //PROTOTYPE for calculating total profit
    bool PrintItems()const; //PROTOTYPE for printing items in vector
    void DisplayWarning(); //PROTOTYPE for displaying a warning
    bool DeleteAnItem(); //PROTOTYPE for deleting an item from the vector
};
#endif /* Items_hpp */

Items.cpp Sale System

#include "Items.hpp"
#include "Item.hpp"
#include <stdio.h>
#include <iostream>
#include <vector>
#include <string>
#include <iomanip>

//This procedure allows the user to type in the name of the item, and if the item exists, will subseqently be deleted.
bool Items::DeleteAnItem()
{
    std::string sNameOfItemToBeDeleted;
    std::cout << "Input name of item that you wish to delete: ";
    std::cin.ignore(); //This ignores anything that is in the cin buffer.
    std::getline(std::cin,sNameOfItemToBeDeleted);
    for (int iCount = 0; iCount < ItemsVec.size();iCount++)
    {
        if (ItemsVec.at(iCount).GetItemName() == sNameOfItemToBeDeleted)
        {
            ItemsVec.erase(ItemsVec.begin() + iCount);
            std::cout << sNameOfItemToBeDeleted << " has been deleted.\n";
            std::cin.clear();
            return true;
        }
    }
    std::cout << "We couldn't find the item you're trying to delete\n";
    return false;
}
bool Items::CheckIfItemExists(std::string sInputName)
{
    for(int iCount = 0; iCount < ItemsVec.size();iCount++)
    {
        if (ItemsVec.at(iCount).GetItemName() == sInputName)
        {

            std::cout << "Try again! There's already an item with that name.\n";
            return false;
        }
    }
    return true;
}
void Items::AddNewItem()
{
    bool bValid = false;
    std::string sInputName;
    double dInputSalePrice = 0;
    int iInputQuantity = 0;
    do{
    std::cout << "Enter information for new item..." << std::endl;
    std::cout << "\tName: ";

    std::cin.clear();
    std::getline(std::cin,sInputName);
    bValid = CheckIfItemExists(sInputName);

    }while(bValid == false);
    bool bSalePriceValid = false;
    do{
        std::cout << "\tSale price: £";
        std::cin >> dInputSalePrice;
        if(!std::cin)
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
            std::cout << "\n";
            std::cout << "Sorry, invalid input input. Try again!\n";
            std::cout << "\n";
        }
        else
        {
            bSalePriceValid = true;
        }
    }while(bSalePriceValid == false);

    bool bQuantityValid = false;
    do{
        std::cout << "\tQuantity sold: ";
        std::cin >> iInputQuantity;
        if(!std::cin)
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
            std::cout << "\n";
            std::cout << "Sorry, invalid input input. Try again!\n";
            std::cout << "\n";
        }
        else
        {
            bQuantityValid = true;
        }
    }while(bQuantityValid == false);

    Item NewItem(sInputName, dInputSalePrice, iInputQuantity);
    ItemsVec.push_back(NewItem);
    std::cout << "=======================================================================\n";
    std::cout << "You've succesfully added a " << "'" << sInputName << "' as an item.\n";
    std::cout << "=======================================================================\n";

}

void Items::GetItemInformation()
{
    bool bValid = false;
    int iInputItemSize = 0;
    std::string sItem;
    do{
        std::cout << "How many items would you like to add?: ";
        std::cin >> iInputItemSize;
        if(std::cin.good())
        {
            if(iInputItemSize < 5)
            {
                std::cout << "\n";
                std::cout << "Try again, number must be greater than 5!\n";
            }
            else
            {
                bValid = true;
            }
        }
        else
        {
            std::cin.clear();
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
            std::cout << "Sorry, invalid input input. Try again!";
            std::cout << "\n";
        }

    } while(bValid == false);


    for (int iCount = 0; iCount < iInputItemSize;iCount++)
    {
        std::cin.ignore();
        AddNewItem();
        std::cout << "\n";

    }
    std::cout << "You've successfully added " << iInputItemSize <<" items\n";

}
int Items::CalculateQuantitySold(){
    int iTotalQuantitySold = 0;
    for (int iCount = 0; iCount < ItemsVec.size();iCount++){
        iTotalQuantitySold += ItemsVec.at(iCount).GetItemQuantity();
    }
    return iTotalQuantitySold;
}

int Items::MostSoldItem()
{
    Item MostSoldItem("",0,0);
    int iIndexOfMostSoldItem = 0;
    for (int iCount = 0; iCount < ItemsVec.size();iCount++)
    {
        if ( ItemsVec.at(iCount).GetItemQuantity() > MostSoldItem.GetItemQuantity() )
        {
            MostSoldItem = ItemsVec.at(iCount);
            iIndexOfMostSoldItem = iCount;
        }
    }
    return iIndexOfMostSoldItem;
}
int Items::LeastSoldItem()
{
    Item LeastSoldItem("",0,0);
    int iIndexOfLeastSoldItem = 0;
    for (int iCount = 0; iCount < ItemsVec.size();iCount++)
    {
        if (ItemsVec.at(iCount).GetItemQuantity() < LeastSoldItem.GetItemQuantity())
        {
            LeastSoldItem = ItemsVec.at(iCount);
            iIndexOfLeastSoldItem = iCount;
        }
    }
    return iIndexOfLeastSoldItem;
}
void Items::DisplayVectorOfItemsAtIndex(int iIndex)
{
    std::cout << "==========================================\n";
    std::cout <<"\tName: " << ItemsVec.at(iIndex).GetItemName() << "\n";
    std::cout <<"\tSale Price: £" << ItemsVec.at(iIndex).GetItemSalePrice() << "\n";
    std::cout <<"\tQuantity Sold: " << ItemsVec.at(iIndex).GetItemQuantity() << "\n";
     std::cout << "==========================================\n";
    std::cout << "\n";
}

double Items::CalculateDifferenceBetweenLeastAndMostSold()
{
    int iDifferenceBetweenMostLeast = 0;
    int iMostSoldIndex = MostSoldItem();
    int iLeastSoldIndex = LeastSoldItem();
    iDifferenceBetweenMostLeast = ItemsVec.at(iMostSoldIndex).GetItemQuantity() - ItemsVec.at(iLeastSoldIndex).GetItemQuantity();
    return iDifferenceBetweenMostLeast;
}
double Items::CalculateTotalSales()
{
    double dTotalSales = 0;
    for (auto Item : ItemsVec)
    {
        dTotalSales += Item.GetItemSale();
    }
    return dTotalSales;
}
double Items::CalculateEstimatedOperationalCost()
{
    double dTotalSales = CalculateTotalSales();
    double dEstimatedOperationalCost = dTotalSales * 0.5;
    return dEstimatedOperationalCost;
}
double Items::CalculateTaxPaid()
{
    double dTaxPaid = CalculateTotalSales() * 0.2;
    return dTaxPaid;
}
double Items::CalculateProfit()
{
    double dTotalProfit = 0;
    double dTotalSales = CalculateTotalSales();
    dTotalProfit = dTotalSales - CalculateTaxPaid() - CalculateEstimatedOperationalCost();
    return dTotalProfit;
}

bool Items::PrintItems()const
{
    int iCountNumber = 1;
    if (ItemsVec.size() == 0)
    {
        std::cout << "There are no items to display.\n";
        return false;
    }
    else
    {
        for (const auto &Item : ItemsVec)
        {
            std::cout << "Displaying information for item " << iCountNumber++ << "...\n";
            std::cout << "==========================================\n";
            std::cout << "\tName: " << Item.GetItemName() << "\n";
            std::cout << "\tSale Price: £" << Item.GetItemSalePrice() << "\n";
            std::cout << "\tQuantity Sold: " << Item.GetItemQuantity() << "\n";
            std::cout << "==========================================\n";
            std::cout << "\n";
        }

    }
    return true;
}
void Items::DisplayWarning()
{
    double dTotalSales = CalculateTotalSales();
    double FivePercentOfTotalSales = dTotalSales * 0.05;
    bool bWarning = false;
    for (int iCount = 0; iCount < ItemsVec.size();iCount++)
    {
        if (ItemsVec.at(iCount).GetItemSale() <= FivePercentOfTotalSales)
        {
            bWarning = true;
        }
        else
        {
            bWarning = false;
        }
    }
    if (bWarning == true)
    {
        std::cout << "Warning! You have products that constitutes less than 5% of sales individually.\n";
    }
    else
    {
        std::cout << "Well done! All your products constitutes to more than 5% of sales individually.\n";
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Which specific version of the language are you targeting? You should specify at most one of the C++11, C++14, and C++17 tags. The contents of a review may vary depending on which language spec you're using. \$\endgroup\$ – 1201ProgramAlarm Jan 26 at 21:42
  • \$\begingroup\$ C++11 I think... \$\endgroup\$ – George Austin Bradley Jan 26 at 22:11
  • \$\begingroup\$ Hello, please don't edit your question to include feedback from answers. This makes answers nonsensical because the points they address no longer exist in the question. See What you may and may not do after receiving answers. \$\endgroup\$ – L. F. Mar 15 at 4:56
4
\$\begingroup\$

To get you started:


Naming

Hungarian notation (e.g., bValid) is not generally used in C++. C++ is a statically typed language, so the compiler will do type checking. Also, the common naming scheme in C++ is:

  • snake_case for variables and functions;

  • CamelCase (or Like_this) for classes; and

  • ALL_CAPS for macros.

You can come up with your own naming scheme if you have a good reason, but sticking to the common one is better in general.

Macro names are not subject to scopes, so avoid common names like Item_hpp (or ITEM_HPP). You can append a random string: ITEM_HPP_h0hCHfEa5Y.

Item and Items are way too similar. Consider using a distinctive name such as Collection instead of Items for clarity.


ContinueOptions

This function should be named confirm_continue because it simply forces the user to press Y and then continue. If the user chooses not to continue, the system says "Please try again." (sad face)

//When called this asks the user how many items they would like to add and the item number has to be greater than 5.
//Through the parameter is passed a vector of items.

??? Not sure what that means ...

cInputCommandPrompt is not a good name because it stores the input rather than the prompt (which is what you show to the user). input is enough.

This is a bit scary:

std::toupper(static_cast<unsigned char>(cInputCommandPrompt))

Technically yes, this prevents UB. I never saw that before (and never wrote that), but it's right the more I think about it. You can declare the variable as unsigned char in the first place. Or just use char ...

"Press ""y"" to continue: " is equivalent to "Press y to continue". You are concatenating three string literals.

#include <cctype> is missing for std::toupper. Also, std::tolower is more commonly used for case-insensitive comparison.

Also, the logic is a bit convoluted. Here's an attempt at simplification:

// requires '#include <limits>'
constexpr auto stream_max = std::numeric_limits<std::streamsize>::max();

template <typename T>
auto& input(std::string_view prompt, T& value) // requires '#include <string_view>'
{
    std::cout << prompt;
    return std::cin >> value;
}

void confirm_continue()
{
    char ch{};
    while (!input("Press 'y' to continue: ", ch) && std::tolower(ch) == 'y') {
        std::cin.clear();
        std::cin.ignore(stream_max, '\n'); // has special meaning
        std::cout << "Please try again.\n";
    }
}

This is arguably clearer. See cppreference for the use of stream_max.


AdditionalMenu

The advertised 2. Search for specific item functionality results in Invalid input. Try again!. At least use something like Coming soon ....

The two kinds of invalid input can be unified somehow.

Similar simplification:

void additional_menu(Items& items)
{
    while (true) {
        int option{};
        if (input("Enter a number from the additional menu: ", option)) {
            switch (option) {
            case 0:
                return;                             // exit additional menu
            case 1:
                items.DeleteAnItem();
                confirm_continue();
                continue;
            case 2:
                std::cout << "Coming soon ...\n\n";
                continue;
            }
        } else {
            std::cin.clear();
            std::cin.ignore(stream_max, '\n');
        }
        std::cout << "Invalid input. Try again!\n";
    }
}

The main function can be simplified analogously.


Item

Only #include necessary headers. You only need <string>.

The Item class doesn't maintain any class invariant, so use an aggregate (which is basically an all-public class) to get rid of the constructor and observer functions:

#include <string>

struct Item {
    std::string name;
    double price;
    int quantity_sold;

    const double sale() const noexcept
    {
        return price * quantity_sold;
    }
};

Now the Item.cpp file can be removed.


Items

As I said before, rename to Collection or something like that. And rename ItemsVec to items.

Comments like //The vector<Items>ItemsVec is private so only member functions/procedures have access to it. are unnecessary because competent programmers are familiar with the basic language constructs.

Now the individual functions:

Delete an item

std::cin.ignore(); ignores one character, not "anything that is left" ;) Use std::cin >> std::ws instead. Consider handling invalid input?

Use std::find_if (requires #include <algorithm>) + lambda to find the item:

bool Items::delete_item()
{
    std::string name;
    std::cout << "Input name of item that you wish to delete: ";
    getline(std::cin >> std::ws, name);

    auto it = std::find_if(items.begin(), items.end(),
                           [&](const Item& item) { return item.name == name; });
    if (it == items.end()) { // not found
        std::cout << "We couldn't find the item you're trying to delete\n";
        return false;
    } else {
        items.erase(it);
        std::cout << name << " has been deleted.\n";
        return true;  
    }
}

Check for existence

It should be ensure_nonexistent based on the return value.

Similarly:

bool Items::ensure_nonexistent(std::string name)
{
    auto it = std::find_if(items.begin(), items.end(),
                           [&](const Item& item) { return item.name == name; });
    if (it == items.end()) {
        return true;
    } else {
        std::cout << "Try again! There's already an item with that name.\n";
        return false;
    }
}

Add new item

The function can be simplified analogously, but note that this:

Item NewItem(sInputName, dInputSalePrice, iInputQuantity);
ItemsVec.push_back(NewItem);

introduces overhead by copying the item. Use move semantics:

Item item{/* ... */};
items.push_back(std::move(item)); // #include <utility>

or simply:

items.push_back(Item{name, price, quantity});

GetItemInformation

The name is deceptive! It's actually add_items.

Calculate total quantity

Make the function const. Use std::accumulate: (#include <numeric>)

int Items::total_quantity_sold() const noexcept
{
    return std::accumulate(items.begin(), items.end(), 0,
        [](int sum, const Item& item) { return sum + item.quantity_sold; });
}

Similarly for CalculateTotalSales.

Most / least sold item

Similarly, use max_element and min_element:

int Items::most_sold_index() const noexcept
{
    auto it = std::max_element(items.begin(), items.end(),
        [](const Item& lhs, const Item& rhs)
        {
            return lhs.quantity_sold < rhs.quantity_sold;
        });
    return it - items.begin();
}
int Items::least_sold_index() const noexcept
{
    auto it = std::min_element(items.begin(), items.end(),
        [](const Item& lhs, const Item& rhs)
        {
            return lhs.quantity_sold < rhs.quantity_sold;
        });
    return it - items.begin();
}

int is actually not the correct type for indexes. It doesn't matter in this case, but std::vector<Items>::size_type is better. Or std::vector<Items>::const_iterator::difference_type ...

Other calculations

Instead of the verbose

double Items::CalculateEstimatedOperationalCost()
{
    double dTotalSales = CalculateTotalSales();
    double dEstimatedOperationalCost = dTotalSales * 0.5;
    return dEstimatedOperationalCost;
}
double Items::CalculateTaxPaid()
{
    double dTaxPaid = CalculateTotalSales() * 0.2;
    return dTaxPaid;
}
double Items::CalculateProfit()
{
    double dTotalProfit = 0;
    double dTotalSales = CalculateTotalSales();
    dTotalProfit = dTotalSales - CalculateTaxPaid() - CalculateEstimatedOperationalCost();
    return dTotalProfit;
}

Do

double Items::estimated_operational_cost() const
{
    return total_sales() * 0.5;
}
double Items::tax() const
{
    return total_sales() * 0.2;
}
double Items::profit()
{
    return total_sales() - tax() - estimated_operational_cost();
}

This is a lot better.


Miscellaneous

Sort #include directives in alphabetical order to ease navigation.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ First of all can I just say how thankful I am that you've taken the time to review my program and give me feedback on it as well fantastic solutions and simplifications. I'm truly indebted to you. I do have a slight problem with: auto it = std::find(items.begin(), items.end(), [&](const Item& item) { return item.name == name; }); I've added #include <algorithm> however it gives me a compiler error I'm not familiar with. What was that you said about + lamba? \$\endgroup\$ – George Austin Bradley Jan 27 at 16:07
  • \$\begingroup\$ What does this mean? No matching function for call to object of type '(lambda at /Applications/Practice.cpp/Practice.cpp/Practice/doodle/Sale System/Sale System/Items.cpp:159:28)' \$\endgroup\$ – George Austin Bradley Jan 27 at 16:19
  • \$\begingroup\$ I'm having problems implementing "Delete an item", "Check for existence", "Calculate total quantity", and "Most / least sold item". It's something to do with "lambda". Do you know the cause of this problem? \$\endgroup\$ – George Austin Bradley Jan 27 at 18:55
  • \$\begingroup\$ @GeorgeAustinBradley Sorry, I made a mistake. The two std::finds should be std::find_if. And the argument to std::accumulate is wrong. Hopefully the interface of STL will get easier to use with the C++20 Ranges library! \$\endgroup\$ – L. F. Jan 28 at 5:17

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.