Creating an inventory using classes

Question

The next assignment in my class is to create a program that allows the user to input an inventory, and then display the total value of the inventory. We are required to create a class with both private and public member functions. I tried to keep it as streamlined as possible but I know there is always room for improvement. Let me know what you think.

I defined the member functions outside of the class by direction of the prof. I personally like seeing it all in one place, but this is an OOP class, so I guess it makes sense to define them outside. The only other direction was that all values have to be positive, hence the do while loops.

#include <iostream>

using namespace std;

class inventory
{
    private:
        int productNum[10];
        int productCount[10];
        double productPrice[10];

    public:
        int locator;
        void retrieve();
        void init();
        void store();

}inv1;

void setItemNumber(int &temp)
{
    do
    {
        cout << "\nPlease enter the item number (if finished please enter '0'): ";
        cout << "\nNumber must be positive: " << endl;
        cin >> temp;        
    } while (temp < 0);
}
void setQuantity(int &temp)
{
    do
    {
        cout << "\nPlease enter the Quantity: ";
        cout << "\nNumber must be positive: " << endl;
        cin >> temp;        
    } while (temp < 0);
}
void setPrice(double &price)
{
    do
    {
        cout << "\nPlease enter the Price: ";
        cout << "\nNumber must be positive: " << endl;
        cin >> price;       
    } while (price < 0);
}

void inventory :: init()
{
    int counter = 0;
    while (counter < 10)
    {
        productCount[counter] = 0;
        productNum[counter] = 0;
        productPrice[counter]= 0;
        counter++;
        locator = counter;
    }
}

void inventory :: store()
{
    int counter = 0, temp = 0;
    double price = 0;
    cout << "\nWelcome to the inventory control system" << endl;
    cout << "When finished please enter '0' for the item number" << endl;
    while (counter < locator)
    {
        setItemNumber(temp);
        if(temp == 0)
        {
            locator = counter;
            return;
        }
        productNum[counter]=temp;
        setQuantity(temp);
        productCount[counter]=temp;
        setPrice(price);
        productPrice[counter]=price;
        counter++;
    }
}

void inventory :: retrieve()
{
    int counter = 0;
    double total = 0;
    cout << "Product Number \t\t Product Count \t\t Price" << endl;
    cout << "_______________________________________________________" << endl;
    while (counter < locator)
    {
        cout << productNum[counter];
        cout << "\t\t\t" << productCount[counter];
        cout << "\t\t\t" << productPrice[counter] << endl;
        total = productCount[counter] * productPrice[counter] + total;
        counter++;
    }
    cout << "Your total inventory is valued at: $" << total << endl;
}

int main()
{

    inv1.init();
    inv1.store();
    inv1.retrieve();

    return 0;
}

Answer 1

int productNum[10];
int productCount[10];
double productPrice[10];

That's not exactly OOP what you did here. A single product is apparently defined by the combination of productNum, productCount and productPrice. So these 3 attributes should have been grouped together in a product class:

class product {
    int productNum;
    int productCount;
    double productPrice;
}

I'll let it to your exercise to decide on the visibility of these attributes, and to add a constructor.

And the inventory class then correspondingly holds a collection of products instead:

product products[10];

Well, except that this isn't exactly a clean way to manage a collection of products in C++ either, better use a dynamically growing data type with checked bounds:

std::vector<product> products;

---

This also brings us to the next point, your use of these two:

int locator;
int counter = 0;

I assume your locator variable was actually supposed to indicate the fill level of the original inventory tracking mechanism. Please try to name it properly the next time. There is also a second problem with locator, you made it public even though any external modification to this variable would break the integrity of your implementation.

---

void inventory::init() {
    ...
}

This method should have been called void inventory::inventory(), which makes it the constructor for the inventory class and is called automatically every time a new inventory is instantiated. Have a read on the RAII principle.

---

void inventory::store();
void inventory::retrieve();

These two have a rather strange signature for what they are called. Your task was to create an inventory class which manages an inventory. No less, no more. What you actually did, was cramming all the input and output logic into that class as well. That's a direct violation of the Separation of Concerns principle.

What that part of the API should look like is simple:

void inventory::store(product p);
double inventory::getTotal();

These methods shouldn't contain any input/output logic, but only the algorithms relevant to storing products, respectively to query all stored products for the grand total.

The input/output belongs into the scope working with the inventory instance, not into that class.

---

class inventory {...} inv1;

That's a global variable you created here. Unless you have a very good reason to do so (and trust me when I say you almost never have), don't. You should have instantiated inventory inside the main() function instead.

---

I defined the member functions outside of the class by direction of the prof. I personally like seeing it all in one place, but this is an OOP class, so I guess it makes sense to define them outside.

Not quite. You should probably be familiar by now with the concept of splitting your source code into header and source files. With headers containing structural information, and source files containing implementations.

In this case, they class definition should have gone into a file called inventory.h and the implementation of the class would have belonged into inventory.cpp.

If take a look back at the section about the global variable inv1, it should strike why that was a mistake.

---

We are required to create a class with both private and public member functions.

You don't have any private methods yet. Just food for thought, in regard to the Separation of Concerns, you could actually structure your API like this:

class inventory {
    private:
        // Actually stores the product
        void storeUnchecked(product p);
        ...
    public:
        // Validates that the product fulfills the requirements
        // If yes, passes down to storeUnchecked()
        bool store(product p);
        ...
}

Since adding a product can obviously fail if validation fails, you want to account for that by allowing your API to signal the success or failure back to the calling location.

Answer 2

Few things I've noticed, let's start from top to bottom:

using namespace std;

While not really wrong, this is often considered bad practice, because it's easy to get into ambiguous situation. The namespace is there for a reason, so you should use it.

class inventory
{
    private:
        int productNum[10];
        int productCount[10];
        double productPrice[10];

    public:
        int locator;
        void retrieve();
        void init();
        void store();
} inv1;

Several problems here:

• Most obviously, this isn't really object oriented. You just crammed your standard non-OOP program in a class.
• Also, rather than working with instances of the class you're only creating a global object inv1, which usually is considered bad practice as well.
• As mentioned in the comments, init() is supposed to be only run once and required for everything else in the class to work. This essentially is the constructor, so it should be implemented as such.
• Many variables you're using have misleading or (in my opinion) completely illogical names or sometimes names not speaking at all. Every variable (or pretty much any element) with a good name doesn't require a comment explaining it, which is always a win. Names such as setPrice are talking and specific, great! Names such as locator or temp are not.
• Also you might want to set for some additional markup to denote variable class members, e.g. using mLocator over locator (despite the name itself), so it's always easy to see that you're accessing a member variable rather than some local one.

 

do
{
    // ...
    cin >> temp;        
} while (temp < 0);

You might have noticed that there's something wrong with your input in case the user does something "wrong", the program will no longer wait for any further input and just continue running (or exit). Rather than explaining this in detail here, you can read about it here and also find possible solutions.

int counter = 0;
while (counter < 10)
{
    // ...
    counter++;
}

While this loop structure is perfectly fine, you essentially expressed a typical for() loop using a while(). You can improve readability by simply using a for() instead:

for (int counter = 0; counter < 10; ++counter)
{
    // ...
}

I'm still not 100% sure what your intention is regarding the member locator. It seems to be changed more often than it has to and the use isn't really clear.

void inventory :: init()
{
    // ...
}

The spacing you're using around the namespace separator is really weird and I haven't seen it done before. I would advice against it, as it makes the whole line harder to read, especially if you're working with a constructor and initializer lists.

Also, one last note: While not mistake or problem directly, personally I tend to start my class names with uppercase characters. This way it's immediately easy for me to see something is a class or struct (uppercase) or some instantiated object/variable (lowercase).

---

Going back to my initial point – since I think that's the important part here –, I think a far better OOP approach would group the properties of your inventory items in their own class. The following is just some quick example how I'd probably do this. This doesn't mean this code is perfect and I'm pretty sure there are also things to criticize in there. Just use it for inspiration or to see what you could do different.

Note: I've written this from scratch without being able to test it right now; there might be tiny mistakes in there! Also since this is a learning experience, try to understand the code; don't just copy and paste, please! :)

class Inventory
{
    // Nesting the Entry class is completely optional
    // but it makes it obvious it's an "Inventory Entry"

    // Note that I'm intentionally inlining the Entry members
    // They're pretty simple and doing so allows the compiler to
    // optimize these out more easily.
    class Entry
    {
        // We can just setup a simple constructor here
        Entry(unsigned int quantity = 0, double value = 0.)
            : mQuantity(quantity), mValue(value) {}

        // Some simple accessors
        // Note that all names are speaking,
        // so I don't have to explain anything
        void setQuantity(unsigned int quantity) { mQuantity = quantity; }
        void setValue(double value) { mValue = value; }
        unsigned int getQuantity() const { return mQuantity; }
        double getValue() const { return mValue; }
        double getTotalValue() const { return mValue * mQuantity; }

    private:
        unsigned int mQuantity;
        double mValue;
    }

    // Here you'd define your accessors again
    // but how about accessing the n-th entry
    // directly using `myInventory[n]`?
    Entry& operator[](unsigned int index)
    {
        // Here I'm cheating a bit to skip error checking
        // the index can't go out of bounds this way.
        // Instead this should probably throw an exception!
        return mEntries[index % scSize];
    }

    // This member retrieves the value of each entry and create a sum
    double getTotalValue() const
    {
        double total = 0.;
        for (unsigned int n = 0; n < scSize; ++n)
            total += mEntries[n].getTotalValue();
        return total;
    }

private:
    // The number of entries on our inventory list
    static const unsigned int scSize = 10;
    // The actual entries in our inventory
    // Note this doesn't need any "init()",
    // because the Entry constructor initializes values
    Entry mEntries[scSize];
}

With this template in place, you can now use this new inventory "system" in a pretty straightforward way (and that's the "magic" of OOP):

// Create a new inventory system
Inventory myInventory;

// Access the third inventory entry:
myInventory[2]; // won't do anything

// Update the quantity for the third entry:
myInventory[2].setQuantity(50);

// Retrieve the total value of my inventory:
double value = myInventory.getTotalValue();

Some more ideas on how this could be expanded:

• Inventory::clear() could reset all quantities and values to 0.
• Inventory::print() could write a nice table with entries and a sum below.
• Inventory::takeStock() could be used to input the entries, but this could also be outside.