Scrutinise my code

[megatron 0]

Looking for brutal criticism, feedback and improvements to my initial OOP design.

My code should be elegant, efficient and easy to understand for beginners.

I am wanting to create an OO system for "items" on say the map, a shop or Player inventory.

I'm not entirely sure about the design of the base class. From what I hear you need to have a common base class between classes so a base class pointer can point to all base classes of derived members. I understand the concept, but do I have to make a base class contain hundreds of pure virtual functions to all different types of derived to avoid slicing? Some light on this issue will be appreciated. It may be because having all game object derived from a single base class may be the wrong way to do things, I just want storing objects in memory to be easy.

/*
 * Object logic tests - 19th December 2014
 * 
 * Initial design of classes and object orientated code
 *
 */

#include <iostream>
#include <string>
#include <vector>

/**********************************************************************\
 * I think all objects should be derived from a single base class
 * The base class will have common data members between all derived
 * classes
\**********************************************************************/

class Object
{
private:
	static unsigned int ObjID; // Keep a global check on how many objects have been created
	const unsigned int MyID;   // The ID of the newly created object
	const std::string Name;    // Every object has a name - Iron Tree Grass House etc etc
public:
	Object(): MyID(ObjID) { ObjID++; }
	Object(const std::string n): MyID(ObjID), Name(n) { ObjID++; }
	const unsigned int& GetID() { return MyID; }
	const std::string& GetName() { return Name; }
	virtual const unsigned int& Use() = 0;
	virtual ~Object() {}
};

unsigned int Object::ObjID = 0;

class Weapon: public Object
{
private:
	const unsigned int Damage;
public:
	Weapon(const std::string n, const unsigned int d): Object(n), Damage(d) {}
	const unsigned int& Use() { return Damage; }
};

class Shield: public Object
{
private:
	const unsigned int Armour;
public:
	Shield(std::string n, unsigned int a): Object(n), Armour(a) {}
	const unsigned int& Use() { return Armour; }
};

// Just some utility functions to help me debug

void DestroyAll(std::vector<Object*>& container)
{
	for(unsigned int i = 0; i < container.size(); i++)
	{
		delete container[i];
		container[i] = nullptr;
	}
}

void CheckAll(const std::vector<Object*>& container)
{
	for(unsigned int i = 0; i < container.size(); i++)
	{
		if(container[i] == nullptr) std::cout << "Object[" << i << "] has been deleted.\n";
		else std::cout << "Object[" << i << "] is active.\n";
	}
}

int main()
{
	std::vector<Object*> objects;
	std::cout << "Creating derived container\n\n";
	objects.push_back(new Weapon("Iron Sword",10));
	objects.push_back(new Weapon("Steel Sword",15));
	objects.push_back(new Shield("Iron Shield",5));

	std::cout << "Displaying Object ID's\n--------------------------\n";
	for(unsigned int i = 0; i < objects.size(); i++)
		std::cout << "object[" << i << "] = " << objects[i]->GetID() << "\n";

	std::cout << "\nTesting methods\n------------------------------\n";
	for(unsigned int i = 0; i < objects.size(); i++)
		std::cout << "object[" << objects[i]->GetName() << "] attribute = " << objects[i]->Use() << "\n";

	std::cout << "\nChecking pointer status\n------------------------\n";
	CheckAll(objects);

	std::cout << "\nDeleting pointers\n";
	DestroyAll(objects);
	
	std::cout << "\nChecking pointer status\n------------------------\n";
	CheckAll(objects);

	std::cout << "\nFinished!\n";
	return 0;
}

[MiiNiPaa]

If you are going to take string by value, make use of default values and move semantic. And do not forget to mark one-argument constructors as explicit:

//Instead of both constructors:
explicit Object(std::string n = ""): MyID(ObjID++), Name(std::move(n)) {}

Avoid manual memory management:

//No checking or deleting nessesary
int main()
{
	std::vector<std::unique_ptr<Object>> objects;
	std::cout << "Creating derived container\n\n";
	objects.push_back(std::make_unique<Weapon>("Iron Sword",10));
	objects.push_back(std::make_unique<Weapon>("Steel Sword",15));
	objects.push_back(std::make_unique<Shield>("Iron Shield",5));
	std::cout << "Displaying Object ID's\n--------------------------\n";
	for(unsigned int i = 0; i < objects.size(); i++)
		std::cout << "object[" << i << "] = " << objects[i]->GetID() << "\n";
	std::cout << "\nTesting methods\n------------------------------\n";
	for(unsigned int i = 0; i < objects.size(); i++)
		std::cout << "object[" << objects[i]->GetName() << "] attribute = " << objects[i]->Use() << "\n";
	std::cout << "\nFinished!\n";
}

[megatron 0]

Thanks for the reply MiiNiPaa, I'll read up on the explicit keyword. Is there anything else you can identify as a design flaw? I'm looking for most input on the class design and the derivation of new classes. Of course weapons and shields with have more attributes than what is shown.

[MiiNiPaa]

Also:
Use default/deleted functions. For example in your case: virtual ~Object() = default;

I would suggest to avoid unnesessary inheritance if possible. If your base class cannot provide usable and safe common interface, it is useless. What is the difference between weapon and armor? What is common? How can you represent them as one entity?

For example you can say that weapons and armor are objects which is equipped to different slots and provide different bonuses.

So your Object can be represented as:

class Item
{
//...
    enum type {WEAPON, ARMOR};
//...
    type slot;
    std::vector<std::shared_ptr<Effect>> effects;
};

You use slot member to determine what it is and where you can equip it; use effects member to determine effects of eqipping

//...
Character::equip(slot s, Item& i)
//...
    i.apply(*this); //Possible double dispatch
//...  

[megatron 0]

Another set of excellent suggestions, I really appreciate the help.

std::vector<std::shared_ptr<Effect>> effects;

Hmm... It appears I need to create objects before the Item object. I will take time to better define my classes. I just fear I am going "too far down" the scale of OOP. I don't know to what scale I have to code at, if that makes sense.


Also,

Use default/deleted functions. For example in your case: virtual ~Object() = default;

What does default do in this case?

[MiiNiPaa]

Exactly the same you did: destructor with empty body. But in this case it would count as default destructible for purpose of different type_traits. It might be not really important here, but it is a good idea to explicitely declare default versions of constructors/destructor/operators as such.