restocking inventory with inheritance

[masterinex]

I am trying to write a function that will restock the inventory ( a vector of shared pointer of "Item") when the number of items for a product sold is lower than 10 . Then it will restock (set) the number to 20.

But the bool restockInventory() method is not working.Hope you can look into it.

"item.hpp"

#include <string>
#include <iostream>

using namespace std;

class Item
{
protected:
	string name_;
	double price_;
	int num_;
public:
	Item() = default;
	Item(string n, double p, int num) : name_(n), price_(p), num_(num) {};
	void to_string()
	{
		cout << "Name: " << name_ << "Price: " << price_ << endl;
	}
	virtual string getName() { return name_; }
	virtual int getNum() { return num_; }
	virtual void setNum(int n) { num_ = n; }
	virtual void dispenseItem() = 0;
	virtual ~Item() {}

};

class Phone : public Item
{
public:
	Phone(string n, double p, int num) : Item(n, p, num) {};
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }
	virtual bool activate() = 0;
	virtual ~Phone() {}
};

class AndroidPhone : public Phone
{
public:
	AndroidPhone(string n, double p, int num) : Phone(n, p, num) {};
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }
	bool activate() override { cout << "Android phone activated\n"; return true; }
	void dispenseItem() override { cout << "Dispensing " << name_ << "from kiosk.\n"; }
	//DEF_BRANDNAME(iphoneT)
};

class iPhone : public Phone
{
public:
	iPhone(string n, double p, int num) : Phone(n, p, num) {};
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }
	bool activate() override { cout << "iPhone activated\n"; return true; }
	void dispenseItem() override { cout << "Dispensing " << name_ << "from kiosk.\n"; }
};

class Case : public Item
{
public:
	Case() = default;
	Case(string n, double p, int num) : Item(n, p, num) {};
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }

};

class AndroidPhoneCase : public Case
{
protected:
	double num_;
public:
	//AndroidPhoneCase(string n, double p) : Case(n, p) {};
	AndroidPhoneCase(string n, double p, int num) : Case(n, p, num) {};
	/*AndroidPhoneCase(string n, double p) 
	{ 
		Item::name_ = n;
		Item::price_ = p;
	}*/
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }
	void dispenseItem() override { cout << "Dispensing " << name_ << "from kiosk.\n"; }

};

class iPhoneCase : public Case
{
protected:
	double num_;
public:
	iPhoneCase(string n, double p, int num) : Case(n, p, num) {};
	int getNum() override { return num_; }
	void setNum(int n) override { num_ = n; }
	void dispenseItem() override { cout << "Dispensing " << name_ << "from kiosk.\n"; }
};


"PhoneManager.hpp"
#include <vector>
#include <string>
#include <memory>
#include "item.hpp"

using namespace std;


class PhoneManager
{
public:
	// using pointer_type = shared_ptr<Item>;
	typedef shared_ptr<Item> pointer_type;
	//typedef shared_ptr<Phone> pointer_type_Phone;
	//typedef shared_ptr<Case> pointer_type_Case;
	typedef enum { iphoneT = 0, AndroidT, iPhoneCaseT, AndroidCaseT };
public:
	vector<pointer_type> v;
	static int numItemSize_;
public:
	PhoneManager();		// create a fully stocked kiosk
						// purchase item will randomly choose an item from the vector and remove it 
						// notify that this item was purchased
	bool purchaseItem();
	// returns a vector with the amount left of each type `
	vector<int> doInventory();
	// if the server person come in, restock the kiosk full
	bool restockInventory(vector<PhoneManager::pointer_type> &);

};




"PhoneManager.cpp"
// Where to put the definitions of the phone manager functions
#include <memory>
#include <iostream>
#include <string>
#include <vector>
#include "PhoneManager.hpp"

using namespace std;

// define the static member property outside the class definition
int PhoneManager::numItemSize_ = 20; // always place in a .cpp

// populate the kiosk with 
PhoneManager::PhoneManager()
{
	// 20
	for (unsigned int i = 0; i < numItemSize_; ++i)
	{
		v.push_back(make_shared<iPhone>("iPhoneX", 1500.00, 20));

		// 20
		v.push_back(make_shared<AndroidPhone>("Google Pixel", 1500.00, 20));
		// 20
		v.push_back(make_shared<iPhoneCase>("Otterbox", 50.00, 20));
		// 20
		v.push_back(make_shared<AndroidPhoneCase>("Lifeskin", 75.00, 20));
	}
	

}

vector<int> PhoneManager::doInventory()
{
	vector<int> inventory(4);
	int IPhoneNum = 0;
	int AndroidNum = 0;
	int IPhoneCaseNum = 0;
	int AndroidCaseNum = 0;

	for (unsigned i = 0; i < v.size(); ++i)
	{
		if (dynamic_cast<iPhone*>(v[i].get()) != nullptr)
		{
			IPhoneNum++;
		}
		else if (dynamic_pointer_cast<AndroidPhone>(v[i]))
		{
			AndroidNum++;
		}
		else if (dynamic_pointer_cast<iPhoneCase>(v[i]))
		{
			IPhoneCaseNum++;
		}
		else if (dynamic_pointer_cast<AndroidPhoneCase>(v[i]))
		{
			AndroidCaseNum++;
		}

	}


	inventory.at(0) = IPhoneNum;
	inventory.at(1) = AndroidNum;
	inventory.at(2) = IPhoneCaseNum;
	inventory.at(3) = AndroidCaseNum;
	return inventory;

}


bool PhoneManager::purchaseItem()
{
		if (!v.empty())
		{
			// generator number from 0~100
			int iRemove = rand() % v.size();
			vector<pointer_type>::iterator it = v.begin() + iRemove;

			// if the element inside the vector is a phone, activate it
			if(auto i = dynamic_pointer_cast<Phone>(*it))
			{
				// if the product type is a phone do activate and dispense 
				i->activate();
				i->dispenseItem();
			}

			else if (auto c = dynamic_pointer_cast<Case>(*it))
			{
				// if the product type is a case just do dispense 
				// dispense the item
				c->dispenseItem();
			}
		
			// remove the item from the vector
			// earse the current product which is pointered by iterator
			v.erase(it);
			// it = v.erase(it);
			return true;
		}
		return false;
}

bool PhoneManager::restockInventory(vector<PhoneManager::pointer_type> &v)
{
	if (true)
	{
		for (unsigned i = 0; i < v.size(); ++i)
		{
			if (dynamic_cast<iPhone*>(v[i].get()) != nullptr && v.at(iphoneT)->getNum() < 10)
			{
				v.at(iphoneT)->setNum(20);
			}
			else if (dynamic_pointer_cast<AndroidPhone>(v[i]) && v.at(AndroidT)->getNum() < 10)
			{
				v.at(AndroidT)->setNum(20);
			}
			else if (dynamic_pointer_cast<iPhoneCase>(v[i]) && v.at(iPhoneCaseT)->getNum() < 10)
			{
				v.at(iPhoneCaseT)->setNum(20);
			}
			else if (dynamic_pointer_cast<AndroidPhoneCase>(v[i]) && v.at(AndroidCaseT)->getNum() < 10)
			{
				v.at(AndroidCaseT)->setNum(20);
			}
		}
	}
	else
	{
			return false;
	}


}


"KioskMain.cpp"
// Simulating the phone kiosk
// Luke
// Jan 5, 2018

#include <iostream>
#include <vector>
#include <memory>
#include <string>
#include <ctime>
#include "PhoneManager.hpp"

using namespace std;

int main()
{
	// seed the random number generator 
	srand (unsigned(time(NULL)));

	// test the default constructor
	PhoneManager pm;

	vector<int> inventory = pm.doInventory();
	cout << "IPhone, Android, IPhoneCase, AndroidCase: " << endl;
	for (auto x : inventory)
	{
		cout << x << endl;
	}

	// simulate the purchasing of 20 items
	for (int i = 0; i < 40; ++i)
	{
		bool sold = pm.purchaseItem();
		if (sold == false)
		{
			cout << "No items sold " << endl;
		}
	}
	inventory = pm.doInventory();
	cout << "IPhone, Android, IPhoneCase, AndroidCase: " << endl;
	for (auto x : inventory)
	{
		cout << x << endl;
	}


	cout << endl;
	cout << endl;
	
	for (unsigned i = 0; i < inventory.size(); ++i)
	{
		if (inventory[i] < 10)
		{
			pm.restockInventory(pm.v);
			cout << "Product need to be restocked. " << endl;

		}
		else
		{
			cout << "Product does not need to be restocked. " << endl;

		}
	}

	cout << endl;
	inventory = pm.doInventory();
	cout << "IPhone, Android, IPhoneCase, AndroidCase: " << endl;
	for (auto x : inventory)
	{
		cout << x << endl;
	}
	
}

[TheIdeasMan]

Hi,

But the bool restockInventory() method is not working.Hope you can look into it.

What is not working, tell us, so we don't have to guess :+)

To better make use of polymorphism, consider having a function that has the same name in all the classes, and performs the increment action.That way you don't have to have an else if chain and dynamic casts : both of which are non scale-able. You can have functions declared to take a base class pointer, and populate the containers with derived class pointers, and the compiler will call the correct function.

[masterinex]

so I should put bool restockInventory() in all the derived classes, but for the purpose of this exercise I want to have it done through PhoneManager.

[TheIdeasMan]

PhoneManager::restockInventory can remain, but each of the derived classes should have its own function to avoid the if else chain you have.

Btw, what kind of nonsense is :

if (true) {
// .. statements
}
else {return false;}

Did you get a warning about function not returning a value?

You still haven't said where you program is going wrong?

[masterinex]

what do u mean ? where do i use this structure:


2
3
4
if (true) {
// .. statements
}
else {return false;}

[dhayden]

At line 232 to erase the purchased item from v. I don't think that's right. Shouldn't you just decrement the number of items?

In other parts of the code, you override getNum() and setNum() in all your classes, but the override versions do the same thing as the base versions. So you don't need to override them at all.

You can use [] instead of at() to improve readability. E.g. v[androidT] instead of v.at(androidT) But note that at() checks to ensure that the index is valid while [] does not.

restockInventory seems unnecessarily complex. Why not just something like:

for (unsigned i=0; i<v.size(); ++i) {
    if (v[i]->getNum() < 10) {
        v[i].setNum(20);
    }
}

Better yet for readability, use a range-based for loop:

for (auto &item : v) {
    if (item->getNum() < 10) {
        item->setNum(20);
    }
}

[Mapler]

@masterinex

What he means is that in code such as

if(true){
    //this block of code will always run since true is always true
    //some stuff
}
else{
    //this code will never run
    return false;
}

your function will always run the first block (which also has no return statements) and will never run the else block. It's been a long time since I've read other people's code but still I recommend giving variables more descriptive names and if for any reason you can't, then it would never hurt to place comments in your code.

[TheIdeasMan]

what do u mean ? where do i use this structure:

if (true) {
// .. statements
}
else {return false;} 

In PhoneManager::restockInventory, but as dhayden says, that function is overly complex. On reflection, I guess the getnum / setnum functions fit the specification of what I was talking about before. Although the names of those functions are fine, I would argue that they shouldn't be part of the public interface (they could be part of a protected interface though). Instead, consider a functions with the name Sell( Item&). For each derived class, this function would carry out the appropriate action for that particular item. To make polymorphism work, one needs one of the following : lvalue reference, smart pointer, raw pointer(prefer not). To have a container of references, one needs to use std::reference_wrapper. For smart pointers, I would recommend a std::unique_ptr here, so you would need to read up about using std::move. shared_ptr isn't appropriate here, who is the ownership being shared with?

Another thing is that if one has get / set functions for each member, the class might as well be a plain struct with just public members. Instead think about what is actually happening to the object: in this case it's easy we are selling it, hence the function name Sell . But it is good to have the get / set functions as protected access. The fact that these are functions make the code stronger. For example, if a get/set function changes where it acquires/puts it's value from (say a file, or SQL DB across a network), then we can change just that function, not everywhere that function is called.

With OOP / polymorphism , one of the goals is to make the code scalable, in other words, one should be able to add new derived classes without having to change all the other code to suit. So that is why naming of functions is important. If one has dynamic_cast and / or an else if chain, it usually means the design is wrong, often because one doesn't understand how a derived pointer/ reference is a valid base class pointer /reference.

Also, consider having a class for the inventory itself. At the moment it is a vector<int> in main, But PhoneManager has a public vector of shared_ptr and the restocking function is PhoneManager. This is confusing. Maybe rename PhoneManager to InventoryManager? Make the vector private. You shouldn't need inventory vector in main.

Each of your derived classes has protected data, this is bad for a couple of reasons: Protected data is just as bad as public data; there is no need to have the same member in each derived class. If you do, then that member should be pushed up the inheritance as high as possible, but it still has to make sense. Derived classes should only have members that specialise it from it's base class. Make the inheritance work for you.

At the moment you have these separate variables: Item::num_ ; AndroidPhoneCase::num_ ; IPhoneCase::num_ . This might the heart of your problem: You set the value of num_ but it is referring to AndroidPhoneCase::num_ not Item::num_ for example.

This code emits a warning about type mismatch:
for (unsigned i = 0; i < inventory.size(); ++i)

The size functions return a type of std::size_t (the largest unsigned type the system has) so you should have:
for (std::size_t i = 0; i < inventory.size(); ++i)

This at least avoids the implicit cast from unsigned long long to unsigned .

But as dhayden says , a range based for loop is better. There is another form which is even safer:

for (auto&& item : inventory) // auto&& is a forward reference, the safest - does lvalue, rvalue, const, non const
{
  // do something with item
}

You could use it for a print function like this:

Print(const Inventory& TheInventory) {
   for (auto&& item : TheInventory) {
       std::cout << item.getAValue() << "\n";
   }

}

Even better, overload the Item operator<< , so you can do this:

Print(const Inventory& TheInventory) {
   for (auto&& item : TheInventory) {
       std::cout << item << "\n";
   }

}

C++17 has structured bindings, you could Google that if you want.

Good Luck !!

Edit:

One more thing:

Don't include the name of the base class in the name of the derived class:

class AndroidPhoneCase : public Case

It is unnecessary clutter that gets out of hand, imagine :

class AndroidPhoneChargerCableAdapterUsb

Instead:

class Phone;
class Android : public Phone;

This brings up another topic: avoid massive inheritance trees. But that is another story involving templates.