0
\$\begingroup\$

Just wondering how my code looks, consider it a "professors input" since I'm not taking classes this semester. Also I'd like to note that this will soon be transitioning to a database approach where I use a MySQL database to get the InventoryItems.

Inventory.h

#pragma once


#include <vector>
#include <iostream>

#include "InventoryItems.h"

class Inventory
{
public:
    Inventory();
    ~Inventory();

    void createNewInventoryItem(std::string itemName = "", unsigned int maxQuantity = 0, unsigned int orderThreshold = 0,
                                                                         double price = 0.0, int quantityInInventory = 0);
    void deleteInventoryItem(int posInVector);
    InventoryItems* getItem(int posInVector);
private:
    std::vector<InventoryItems*> m_inventory; //NOTE: does not use a smart pointer, instead the pointer is deleted from the heap in the deleteInventoryItem function
                                              //      if deleteInventoryItem is not called at the end of the inventory objects life cycle the inventory objects deconstructor will
                                              //      loop through all elements in m_inventory and call deleteInventoryItem for each item
};

Inventory.cpp

#include "Inventory.h"

Inventory::Inventory()
{
    std::cout << "Inventory created" << std::endl;
}

Inventory::~Inventory()
{
    for (int i = 0; i < m_inventory.size(); i++) {
        deleteInventoryItem(i);
    }
    std::cout << "Inventory destroyed" << std::endl;
}

void Inventory::createNewInventoryItem(std::string itemName, unsigned int maxQuantity, unsigned int orderThreshold,
                                                                             double price, int quantityInInventory)
{
    InventoryItems* newItem = new InventoryItems;
    newItem->createInventoryItem(itemName, maxQuantity, orderThreshold, price, quantityInInventory);
    m_inventory.emplace_back(newItem);
}

void Inventory::deleteInventoryItem(int posInVector)
{
    delete m_inventory.at(posInVector);
    m_inventory.erase(m_inventory.begin() + posInVector);
}

InventoryItems* Inventory::getItem(int posInVector)
{
    return m_inventory.at(posInVector);
}

The inventory class will really come into it's own, this is just a proof of concept before I incorporate my database (I.E. inventoryItems is acting as a place holder for the DB and queries, hence me not including it here).

\$\endgroup\$
2
  • 1
    \$\begingroup\$ (Your lines are too wide. (The reason to set e.g. newspapers in multiple columns is a limit to the eyes successfully jumping to the beginning of the next line. Not much of a problem given automatic text wrapping. More so with code viewers/editor that typically don't, and have a hard time with comment (and code) layout if&where they do.) I dislike horizontal scrolling.) \$\endgroup\$
    – greybeard
    Feb 28 at 9:42
  • \$\begingroup\$ This looks irritatingly close to hypothetical code, please (re-)visit What topics can I ask about here? \$\endgroup\$
    – greybeard
    Feb 28 at 9:44
4
\$\begingroup\$

Overview

It's a mistake to be using RAW pointers you should definitely be using some form of smart pointer here.

It does two things for you:

  • It will greatly simplify your code.
  • It will remove the bugs !!!

If you have owned RAW pointers in your class then you MUST implement the rule of three. You do (have owned RAW pointers) but your don't (implement the rule of three). As a result your code is broken.

{
    Inventory   a;
    a.createNewInventoryItem("Wine");

    Inventory   b(a); // You did not implement the copy constructor
                      // nor did you disable it, so the default version
                      // is generated. So this copy is allowed.
                      //
                      // Both a and b have a copy of the same RAW pointer
                      // internally.
}
// b is destoryed (which destroys the pointer)
// a is destroyed (which also destroys the pointer) but that is UB as
// it has already been destroyed.

There are several other things that go wrong but you get the drift.

If you had used std::unique_ptr<> this would have prevented the Inventory object from being copied. If you had used std::shared_ptr<> copying would have been allowed (not sure if the semantics would be correct).


I would point out that you only need to store pointers and have dynamic allocation if you have polymorphic types. Otherwise you can simply store the object directly in the container:

//  This will handle most situations.
std::vector<InventoryItems>    m_items;
                 ///     ^^^ note: No pointer

You do need pointers for polymorphic types:

class Animal { /* Stuff */}
class Cat: public Animal { /* Stuff */}
class Dog: public Animal { /* Stuff */}

std::vector<Animal*>            m_items;
// Here we need pointers because we may store a Cat or a Dog
// So the object stored is not an Animal and we don't know how
// large it is at compiler time.

Interface Design:

class Inventory
{
public:
    void createNewInventoryItem(/*Stuff*/);
    void deleteInventoryItem(int posInVector);
    InventoryItems* getItem(int posInVector);
};

Sure you have a create/delete here and a get.

But when you create an item you don't know where in the container it is. So you can not directly get it back. You could use something about the input parameters that you could reuse (like the item name) or I would expect the create to return some identifier that can be used by the delete and get.

Alternatively your interface simply displays all the items and then you manipulate it from there. To do this you need to loop over all the items. But how many items do you have to loop over? In this case I would expect the class to provide some way to know the range of items available. It could be a size() method or alternatively you could provide iterators of the class so you know how to loop over the inventory.


Representation of the Inventory:

I can create two inventory items that have the same name:

    Inventory   a;
    a.createNewInventoryItem("Wine");
    a.createNewInventoryItem("Wine");

There are now two wine items in the inventory. This does not make sense to me. When you add a new item you should check to see if it already exists. If it does then you need to combine the data (or throw an error). You can do this with std::vector but there are other containers that will help you (maybe std::map).

Code Review

    void createNewInventoryItem(std::string itemName = "", unsigned int maxQuantity = 0, unsigned int orderThreshold = 0, double price = 0.0, int quantityInInventory = 0);

You can create an inventory item without specifying anything! If you pass no parameters it still works and you get an item with no name and no value.


When you create an inventory item it does not return anything. So how do you know what item to delete?

    void deleteInventoryItem(int posInVector);

You can randomly delete items (but there is no understanding of what item will be deleted (unless you get every item first and examine it).

Personally I would index items by their itemName. I would then allow both getItem() and deleteInventoryItem() to work by passing the name.


Don't return a pointer from a get(). I would return a reference to the object. Just because you store it as a pointer does not mean you need to expose that fact to the users of your class.

    InventoryItems* getItem(int posInVector);

Don't use std::endl here; prefer to use "\n" instead.

    std::cout << "Inventory created" << std::endl;

The difference between these two is that std::endl in addition to outputting the newline character it forces the stream to flush. There is no need to flush the stream as this happens automatically when it needs to happen. The problem with forcing a manual flush is that it can be inefficient and cause the application to slow down if there is to many flushes.

If you look at questions on SO asking why writing to standard output in C++ is so slow the actual solution is simply to stop using std::endl and allow the stream to flush itself (and the code magically speeds up).


Sure you can loop like this:

    for (int i = 0; i < m_inventory.size(); i++) {
        deleteInventoryItem(i);
    }

But I would note that this is broken because you call deleteInventoryItem() which not only deletes the item but removes it.

Imagine you have a vector with 4 items.

  • You call delete on item 0. This deletes the item and removes it. Thus moving everything in the vector down one item (new length is 3)
  • You increment i (i is now 1)
  • You call delete on item 1. This deletes the item and removes it. Thus moving everything in the vector down one item (new length is 2)
  • you increment i (i is now 2)
  • You exit the loop.

You only deleted two items.

But a vector is a range. So you can use the range based loop:

    for (auto& item: m_inventory) {
        delete item;
    }

    m_inventory.clear();

Why do you have a two phase creation of a InventoryItems?

    InventoryItems* newItem = new InventoryItems;
    newItem->createInventoryItem(itemName, maxQuantity, orderThreshold, price, quantityInInventory);

You should pass all those parameters to the constructor.


One thing I would add is that you don't know the number of items in the inventory. So you just have to loop over them until you get an exception thrown. It may be worth adding a size() function so you can get the size so you know the limit of the number of items.


You don't even need to use pointers here. There is no polymorphic objects involved. You should simply have used normal objects in the vector and it would have worked as expected.


Don't use unsigned integer types. In most situations they will cause you a lot of headaches. If that extra bit is something you need then you are already at the boundary of your system and should upgrade the integer type to the next larger type to get a bunch more bits.

Save the unsigned integer types for flags.


Don't use double for monetary units. Not all values can be represented exactly and you are going to run into rounding errors when you start adding things up.

Rather use an integer type. If you are using dollars in a double then simply use an integer but store the number of cents.


Potential Solution

How I would do it:

#include <map>
#include <iostream>

#include "InventoryItems.h"

class Inventory
{
    using Container = std::map<std::string, InventoryItems>;
public:
        Inventory();
    
        void createNewInventoryItem(std::string itemName,
                                    int maxQuantity = 0,
                                    int orderThreshold = 0,
                                    int price = 0,
                                    int quantityInInventory = 0);

        void deleteInventoryItem(std::string const& itemName);
        InventoryItems& getItem(std::string const& itemName);

        
        using iterator = typename Container::iterator;

        iterator begin() {return m_inventory.begin();}
        iterator end()   {return m_inventory.end();}

         
    private:
        Container    m_inventory;
};

Inventory::Inventory()
{
    std::cout << "Inventory created" << std::endl;
}

void Inventory::createNewInventoryItem(std::string itemName,
                                       int maxQuantity,
                                       int orderThreshold,
                                       int price,
                                       int quantityInInventory)
{
    auto find = m_inventory.find(itemName);
    if (find == itemName.end()) {
        m_inventory.emplace_back(std::piecewise_construct,
                                 std::forward_as_tuple(itemName),
                                 std::forward_as_tuple(maxQuantity, orderThreshold, price, quantityInInventory));
    }
    else {
        // I don't know how your inventory works.
        // This is just s guess/suggestion.
        find->second.setMax(std::max(maxQuantity, find->second.getMax());
        find->second.setOrderThreshold(std::max(orderThreshold, find->second.getOrderThreshold());
        find->second.setPrice(price);
        find->second.increaseInventory(quantityInInventory);
    }
}

void Inventory::deleteInventoryItem(std::string const& itemName)
{
    m_inventory.erase(itemName);
}

InventoryItems& Inventory::getItem(std::string const& itemName)
{
    auto find = m_inventory.find(itemName);
    if (find == m_inventory.end()) {
        throw std::outofbound("Or something like that");
    }
    return find.second;
}
\$\endgroup\$
1
  • \$\begingroup\$ Wow thank you for the very detailed response! I can see I still have quite a bit to learn and improve on in my programming ability. So the big thing I got from this is that classes should be entirely self contained and (for lack of a better word) self governing? I.e. the class can entirely take care of itself and should not piggy back off of other functions/classes? \$\endgroup\$ Feb 28 at 20:33
1
\$\begingroup\$

In addition to Martin's thorough answer, one small point:

The header uses std::string but does not include <string>. Conversely, it includes <iostream> which it does not use (and which can be moved to the implementation file).

\$\endgroup\$
2
  • \$\begingroup\$ <string> is included in <iostream>. \$\endgroup\$ Mar 8 at 6:33
  • \$\begingroup\$ On your platform, perhaps. For portable code, we can't assume that (unless you can point to a section of the standard that states otherwise). As far as I can tell <iostream>, is specified to include <ios>, <streambuf>, <istream>, <ostream> and no others. \$\endgroup\$ Mar 8 at 7:36

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.