Sign in to follow this  

[C++] 1 level of inheritance problems

This topic is 3454 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hi, I'm making a text RPG. I decided to begin from the easy ones and make the weapon class. I want it to be abstract. Then I have a sword class, which is inherited from weapon. Finally, I have a dagger class which is inherited from sword. Weapon -> Sword -> Dagger I want both the weapon and the sword classes to be abstract. I'm doing something like this.. Weapon.h
#ifndef WEAPON_H
#define WEAPON_H

class Weapon
{
public:
	Weapon( int health = 100, int power = 1, int value = 0, int weight = 1 );
	~Weapon();
	virtual void viewAttributes() const = 0;

protected:
	int* m_pHealth, *m_pPower, *m_pValue, *m_pWeight;
};

#endif

Weapon.cpp
#include "Weapon.h"

Weapon::Weapon(int health, int power, int value, int weight)
{
	m_pHealth = new int( health );
	m_pPower = new int( power );
	m_pValue = new int( value );
	m_pWeight = new int( weight );
}

Weapon::~Weapon()
{
	delete m_pHealth;	m_pHealth = 0;
	delete m_pPower;	m_pPower = 0;
	delete m_pValue;	m_pValue = 0;
	delete m_pWeight;	m_pWeight = 0;
}

Sword.h
#ifndef SWORD_H
#define SWORD_H

class Weapon;

class Sword : public Weapon
{
public:
	Sword( int health = 100, int power = 1, int value = 0, int weight = 1 );
	~Sword();
	virtual void viewAtrributes() const = 0;
};

#endif

Sword.cpp
#include "Weapon.h"
#include "Sword.h"

Sword::Sword(int health, int power, int value, int weight ): Weapon( health, power, value, weight )
{
}

Dagger.h
#ifndef DAGGER_H
#define DAGGER_H

class Weapon;
class Sword;

class Dagger : public Sword
{
public:
	Dagger( int health = 100, int power = 1, int value = 0, int weight = 1 );
	virtual void viewAttributes() const;
};

#endif

Dagger.cpp
#include <iostream>

#include "Weapon.h"
#include "Sword.h"
#include "Dagger.h"

Dagger::Dagger(int health, int power, int value, int weight): Sword( health, power, value, weight )
{
}

void Dagger::viewAttributes() const
{
	std::cout << "Your dagger's attributes:\n"
		<< "Health: " << *m_pHealth << " %\n"
		<< "Power: " << (*m_pHealth) * (*m_pPower) << "\n"
		<< "Value: " << *m_pValue << " gold\n"
		<< "Weight: " << *m_pWeight << " kilograms\n"
		;
}

main.cpp
#include <iostream>

#include "Weapon.h"
#include "Sword.h"
#include "Dagger.h"

int main()
{
	Weapon* w = new Dagger( 100, 10, 84, 12 );
}

--------- First, I want to know if this is good design (I mean the whole idea of how objects relate). If not, what should I do? Another question is: I had to include 4 files in main. Is there to way to reduce that number; because it will increase when I add more types of weapons/swords etc. So, I'm getting some errors: 1>------ Build started: Project: PROJECT1, Configuration: Debug Win32 ------ 1>Compiling... 1>main.cpp 1>f:\my documents\visual studio 2005\projects\project1\project1\main.cpp(9) : error C2259: 'Dagger' : cannot instantiate abstract class 1> due to following members: 1> 'void Sword::viewAtrributes(void) const' : is abstract 1> f:\my documents\visual studio 2005\projects\project1\project1\sword.h(11) : see declaration of 'Sword::viewAtrributes' 1>Build log was saved at "file://f:\My Documents\Visual Studio 2005\Projects\PROJECT1\PROJECT1\Debug\BuildLog.htm" 1>PROJECT1 - 1 error(s), 0 warning(s) ========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ========== Thanks in advance and sorry for the long post.

Share this post


Link to post
Share on other sites
You have a typo: viewAtrributes

Other than that, I agree with the others. As-is, a dagger has no special qualities over a sword, other than the name. Adding a name and or description member to weapon, and I see no reason for any derived classes.

Also, don't use dynamic allocation of the members, that is needlessly complex:

class Weapon
{
public:
Weapon( int health, int power, int value, int weight, const std::string &name );

// Make destructors virtual if the class will be a super class
// however, you may not need one
// ~Weapon();
void viewAttributes() const;

private:
int health, power, value, weight;
std::string name;
};

// weapon.cpp

Weapon::Weapon(int health, int power, int value, int weight, const std::string &name)
:
health(health),
power(power),
value(value),
weight(weight),
name(name)

{
}

void Weapon::viewAttributes() const
{
std::cout << "Your " << name << "'s attributes:\n"
<< "Health: " << health << " %\n"
<< "Power: " << (health * power) << "\n"
<< "Value: " << value << " gold\n"
<< "Weight: " << weight << " kilograms\n";
}

Share this post


Link to post
Share on other sites
Quote:
Also, don't use dynamic allocation of the members, that is needlessly complex:


At least for primitive data types :) Dynamic allocation of members can occasionally be useful, i.e if you want to make your object out of 'components' which you want to behave polymorphically.

Edit :-
I couldnt previously think of a good example to reinforce this. However now I have :)...say you wanted to make your swords upgradable, then if inside your sword object you have a pointer to a Handle object and you also have a pointer to a Blade object then you could swap and change your blades and handles to make whatever kind of sword you liked at runtime :)

[Edited by - chipmeisterc on May 15, 2008 12:09:12 PM]

Share this post


Link to post
Share on other sites
You may want to re-name your Sword class to be a Blade class. It makes sense since a Blade is-a Weapon, and a Dagger is-a Blade. The Blade class could cover things like the blade losing its edge, re-sharpening the blade, the blade breaking, etc. Your breakdown of your weapons seem like a good idea to me thus far.

I also agree with rip-off regarding the dynamic allocation.

Share this post


Link to post
Share on other sites
Beginners are often in the habit of wildly overusing inheritance. This is usually because they don't fully understand the power of composition-based object specialization. Inheritance is difficult to get right the first time, and getting inheritance wrong is the number one thing you can do to screw up early on in a manner that only becomes evident much later. In your case, I would strongly advise against having this multi-level inheritance strategy. It looks fun and elegant now, but please believe me that later it will bite you in the ass and never let go.

BTW, why aren't you including Sword.h in Dagger.h?

Share this post


Link to post
Share on other sites
Alright. I have corrected the typo and it runs fine now.

As Mantear suggested, I will rename the Sword class to Blade.

Now, about the Dagger class...Well, I did it in order to have different types of Swords (Blades now). I am thinking of, like rip-off said, to delete the Dagger class and have a string object in the Blade class to distinguish the types. Maybe having a vector of strings with all the names in..picking a random one in some circumstances and check in a function the blade type to set its attributes. Something like this maybe:

void setAttributes( Sword* const b )
{
if( b->type == "Dagger" )
{
b->m_pHealth = 1;
/*....*/
}
}



Regarding the above code, it might be a good idea not to have default arguments in the constructor..but having pre-set attributes for each weapon/blade type.

-------------------------------------------------------------------------------

Actually, that was what I intented to do. Like saying:
Weapon* myDagger = new SamuraiDagger(); or something like this.

But this will need even more inheritance... My question now is: Why having multiple levels of inheritance will be a problem?

Edit: why having my data members on the heap isn't good? Isn't it supposed to be more efficient?

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
Actually, that was what I intented to do. Like saying:
Weapon* myDagger = new SamuraiDagger(); or something like this.

But this will need even more inheritance... My question now is: Why having multiple levels of inheritance will be a problem?


It's added complexity. You have to be sure that the benefits it gives you outweigh the complexity.

Personally, I think that since most weapons have exactly the same behaviour, and only differ in quantitative terms, they should be all one class.

Weapon* myDagger = new Weapon(TYPE_DAGGER, "SamuraiDagger"); perhaps.

Quote:
Edit: why having my data members on the heap isn't good? Isn't it supposed to be more efficient?


Quite the opposite. You've doubled the memory requirement (since you now need a pointer as well as the original value) and have to spend time allocating and deallocating memory.

Share this post


Link to post
Share on other sites
Creating a new class is only necessary when that new class is adding significant new functionality or data. A samurai sword doesn't do anything differently to a sword, and it doesn't have any particularly different properties to a sword - the thing that's different is the data.

In fact, look at the Sword and Dagger classes in your first post. They both have no new member variables. Sword does literally nothing - you've just got the class there as a way of encoding values for the health/power/value/weight. Dagger's the same, except that it has a viewAttributes method - which, except for the word 'dagger' in it, would not look out of place in the base Weapon class.

The primary disadvantage to this is that whenever you want to change the details for a weapon, or add/delete a weapon, you have to rebuild your code. It also means that code changes to the base 'weapon' class might have to be accounted for by updating every individual other class...

The less code you write, the less opportunity there is for you to write it incorrectly.

It would be better if you went for a data-driven approach. First, you extend the 'Weapon' class to include the name of the weapon, and the code for displaying it, like this:


class Weapon
{
std::wstring name;
int health, power, value, weight;
public:
void displayAttributes() const
{
std::cout << name << ": Attributes:" << std::endl;
std::cout << "Health: " << health << " %" << std::endl;
std::cout << "Power: " << health * power << std::endl;
std::cout << "Value: " << value << " gold" << std::endl;
std::cout << "Weight: " << weight << " kilograms" << std::endl;
}


Then we'll extend the class to be able to load its data from a file, like so:

static Weapon* FromFile(ifstream& input)
{
Weapon* wp = new Weapon();
getline(input, wp->name);
input >> wp->health >> wp->power >> wp->value >> wp->weight;
}
};


FromFile will read a single weapon from a text file that looks something like:

Samurai Sword
100 20 5 2
Dagger
30 5 1 1

So you can define all the weapons, and tweak them all, just by editing that file. You don't even need to rebuild the game, you just run it with the new file.

Lastly, it would be convenient that once you've loaded a weapon you can store it somewhere useful, and, given that these are 'templates' for weapons, create instances of each weapon. Let's create a WeaponLibrary:


class WeaponLibrary
{
private: std::map<std::wstring, Weapon*> weapons;
protected: WeaponLibrary() {}
public: ~WeaponLibrary()
{
for(std::map<std::wstring, Weapon*>::iterator it = weapons.begin(); it != weapons.end(); ++it)
{
delete (*it).second;
}
}
public: static WeaponLibrary* FromFile(const std::string& filename)
{
WeaponLibrary* result = new WeaponLibrary();
ifstream infile(filename);
while(!infile.eof())
{
Weapon* wp = Weapon::FromFile(infile);
result->weapons[wp->name] = wp;
}
return result;
}
public: Weapon* CreateByName(const std::wstring& name)
{
Weapon* prototype = weapons[name];
return new Weapon(*prototype);
}
};


WeaponLibrary is created by calling WeaponLibrary::FromFile, and giving it the name of the file storing all the weapon data. It loads in all the weapons, creating an instance of each one. When you ask it to give you a new weapon, you give it the name of the 'template' you want to use; it finds that weapon, and then creates a copy of it which it returns to you.

Share this post


Link to post
Share on other sites
First, I want to thank you.

I guess my design was crap for a text game.

That's how it is now:

Weapon.h

#ifndef WEAPON_H
#define WEAPON_H

class Weapon
{
friend std::ostream& operator << ( std::ostream& os, const Weapon* const w );

public:
Weapon( int health = 100, std::string type = "IRON_SWORD" );
void setWeaponAttributes( const std::string& name = "IRON_SWORD" );

private:
std::string m_Type;
int m_Health, m_Power, m_Value, m_Weight;
};

#endif



Weapon.cpp

#include <iostream>
#include <fstream>
#include <string>

#include "Weapon.h"

Weapon::Weapon( int health, std::string type ): m_Health( health )
{
setWeaponAttributes( type );
}

void Weapon::setWeaponAttributes( const std::string& name )
{
std::ifstream input;
input.open( "WEAPON_TYPES.txt" );
std::getline(input, name);
input >> m_Power >> m_Value >> m_Weight;
}



I have made a text file to read sword types and obtain their attributes. I get an error though.

1>------ Build started: Project: PROJECT1, Configuration: Debug Win32 ------
1>Compiling...
1>Weapon.cpp
1>f:\my documents\visual studio 2005\projects\project1\project1\weapon.cpp(16) : error C2664: 'std::basic_istream<_Elem,_Traits> &std::getline<char,std::char_traits<char>,std::allocator<_Ty>>(std::basic_istream<_Elem,_Traits> &,std::basic_string<_Elem,_Traits,_Ax> &)' : cannot convert parameter 2 from 'const std::string' to 'std::basic_string<_Elem,_Traits,_Ax> &'
1> with
1> [
1> _Elem=char,
1> _Traits=std::char_traits<char>,
1> _Ty=char,
1> _Ax=std::allocator<char>
1> ]
1> and
1> [
1> _Elem=char,
1> _Traits=std::char_traits<char>,
1> _Ax=std::allocator<char>
1> ]
1> Conversion loses qualifiers
1>Build log was saved at "file://f:\My Documents\Visual Studio 2005\Projects\PROJECT1\PROJECT1\Debug\BuildLog.htm"
1>PROJECT1 - 1 error(s), 0 warning(s)
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========


It refers to the line: std::getline(input, name);

main.cpp

#include <iostream>
#include <string>

#include "Weapon.h"

std::ostream& operator << ( std::ostream& os, const Weapon* const w );

int main()
{
Weapon* w = new Weapon( 100, "IRON_SWORD" );

std::cout << w;

system("PAUSE");
return 0;
}

std::ostream& operator << ( std::ostream& os, const Weapon* const w )
{
os << "Type: " << w->m_Type << "\n"
<< "Power: " << (w->m_Health) * (w->m_Power) / 100 << "\n"
<< "Health: " << w->m_Health << "%\n"
<< "Value: " << w->m_Value << " gold\n"
<< "Weight: " << w->m_Weight << " kilos\n\n"
;

return os;
}



I will have another file to store the player's swords and other items (maybe in another file). Also I want to know the inventory of every NPC (or I'm I looking to far?). Should I have a text file for each NPC or one for all of them?

Share this post


Link to post
Share on other sites
The error is from you trying to getline() into the argument name, which is a const reference, thus cannot be modified. The whole setWeaponAttributes() method doesn't make much sense to me. Perhaps you were supposed to getline(input,m_Type)?

Share this post


Link to post
Share on other sites
What are you really trying to do when you read from the file? Are you trying to find a line of the file that matches the "type" of the weapon you're creating, and then read the corresponding data and set it?

Because what you're trying to do right now is read the first line of the file, and overwrite the "type" with it. The compiler complains because you explicitly said (by writing 'const std::string& name') that you didn't want to overwrite that value.

What does your file contain right now?

Share this post


Link to post
Share on other sites
When you really think about parameterizing weapon types, you come to realize that they fit into very few broad catagories, and beyond that, they are best parameterized on things such as name, range, damage, rate of fire, etc.

Some of the broad categories include:
  • Melee Weapons (up-close, slicing, stabbing, bashing weapons.)
  • Ranged Weapons (long-range ballistic weapons, such as arrows, machine guns or rifles.
  • Smart Weapons (fire and forget weapons with complex behavior, such as mines or guided projectiles.)


Other things, such as a tank or AA gun are best modeled, IMO, as objects which have a weapon through composition, but that the player is able to interact with in the usual ways -- the player doesn't "have" an AA gun or tank, but they can operate them.

Even these broad categories are somewhat arbitrary and could be done away with in some designs, depending on how parameterized you make them.

Share this post


Link to post
Share on other sites
Quote:
Original post by Zahlman
What are you really trying to do when you read from the file? Are you trying to find a line of the file that matches the "type" of the weapon you're creating, and then read the corresponding data and set it?


Yeah, that's what I'm trying to do.

Quote:
What does your file contain right now?


IRON_SWORD
3 10 10
IRON_DAGGER
2 9 3

Share this post


Link to post
Share on other sites
I think I got it working!

Weapon.cpp

#include <iostream>
#include <fstream>
#include <string>

#include "Weapon.h"

Weapon::Weapon( int health, std::string type ): m_Health( health )
{
setWeaponAttributes( type );
}

void Weapon::setWeaponAttributes( const std::string& name )
{
std::ifstream input;
input.open( "WEAPON_TYPES.txt" );

std::string text;

while( !input.eof() && text != name ) // while we haven't reached the end of the file and haven't found the name
{
input >> text; // get the name

if( text == name ) // check if it's what we are looking for
{
m_Type = text;
input >> m_Power >> m_Value >> m_Weight;
break;
}
else // if it's not, give negative values to make me understand the name wasn't found
{
m_Type = "WRONG_TYPE";
m_Power = -1; m_Value = -1; m_Weight = 1;
}
}
input.close();
}

:)

Share this post


Link to post
Share on other sites
Looks alright, but now you're skimming the same file multiple times. It would arguably be better to move the functionality in setWeaponAttributes out of the Weapon class and into a controller class such as WeaponFactory. You then open the data file only once, and generate a template for each weapon (... you need in a particular level for example).

Other than that, it looks like you listened well to some good advice given. Cheers!

EDIT: using said controller class, you can construct your weapons in multiple ways, two of which are through construction of the object; and through property setters:


struct WeaponDesc
{
int Power;
int Value;
int Weight;
};

Weapon::Weapon( const std::string & type, const WeaponDesc & desc )
{
m_Type = type;
m_Power = desc.Power;
m_Value = desc.Value;
m_Weight = desc.Weight;
}


and


Weapon::Weapon( const std::string & type )
{
m_Type = type;
m_Power = -1;
m_Value = -1;
m_Weight = -1;
}

Weapon::SetPower( int Power )
{
m_Power = Power;
}


(and so on for each property)

The advantage of the first one is that data is immutable once the weapon has been created.

Share this post


Link to post
Share on other sites
Quote:
Original post by Todo
Looks alright, but now you're skimming the same file multiple times. It would arguably be better to move the functionality in setWeaponAttributes out of the Weapon class and into a controller class such as WeaponFactory. You then open the data file only once, and generate a template for each weapon (... you need in a particular level for example).


Yeah, that's what I was getting at with my WeaponLibrary example.

Share this post


Link to post
Share on other sites
So, because I open the file multiple times (every time I instansiate (wrong spelling) an object), you recommend me to open it at the beginning of the game and load everything, then read what I need. Correct?

If yes, what about if I had a vector of Weapon(s) that I dynamically allocate in the beginning? Maybe I could have it private to the WeaponFactory class and just make those classes friends.

And again, thanks for the help.

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
So, because I open the file multiple times (every time I instansiate (wrong spelling) an object), you recommend me to open it at the beginning of the game and load everything, then read what I need. Correct?
Yes, correct.

Quote:
If yes, what about if I had a vector of Weapon(s) that I dynamically allocate in the beginning?
Sure, if you're going to load in some unknown number of weapons then you'll need a container to store them in. std::vector would be perfectly suitable for that, though you might find std::map more useful.

Quote:
Maybe I could have it private to the WeaponFactory class and just make those classes friends.
That, on the other hand, sounds like a bad idea. 'friend' is a keyword that you should only find yourself using in very extenuating circumstances...

The approach I'm espousing here is called the 'Prototype pattern.' The idea is that the WeaponFactory has one instance of every type of weapon in its container, and when you ask it to create a new weapon, it does so by making a copy of the one it's got and returning the copy. It's holding a collection of 'prototype' weapons and manufacturing real ones by copying the prototype. As such, nobody should ever have direct access to the WeaponFactory's collection of prototype weapons - you only need some method like "CreateWeapon" that takes the name of a prototype weapon, finds that weapon, makes a copy of it and returns the copy.

Personally, I don't like the name WeaponFactory - I prefer WeaponLibrary - because there may be other functionality you want to build in, e.g. the ability to search through the weapon prototypes for any weapon with a weight that is < 5.

Share this post


Link to post
Share on other sites
Alright, I'm having a bit of a problem here.

Ok, I have made the WeaponLibrary class

WeaponLibrary.h

#ifndef WEAPONLIBRARY_H
#define WEAPONLIBRARY_H

class Weapon;

class WeaponLibrary
{
public:
WeaponLibrary(); //default constructor
Weapon* CreateWeapon( std::string name );

private:
std::vector< Weapon* > m_WeaponLibrary;
}

#endif




By the way, is there a way to restrict me so I can make only one of the above object? Like when I try to make 2 or more, I get an error, or nothing happens.

Now, here where the problem is:
WeaponLibrary.cpp

#include &lt;fstream&gt;
#include &lt;vector&gt;
#include &lt;string&gt;

#include "Weapon.h"
#include "WeaponLibrary.h"

WeaponLibrary::WeaponLibrary()
{
//load all the Weapons

std::ifstream input;
input.open( "WEAPON_TYPES.txt" );

if( !input ) // if the file could not be loaded/found
std::cout &lt;&lt; "Error opening \"WEAPON_TYPES.TXT\"\n";

std::string name;
int power, value, weight;

while( !input.eof() )
{
// get the weapon's name and other attributes
input &gt;&gt; name;
input &gt;&gt; power &gt;&gt; value &gt;&gt; weight;

//make a Weapon object
Weapon* w = new Weapon(
}

input.close();
}

Weapon* WeaponLibrary::CreateWeapon( std::string name )
{
//not done it yet
}




while( !input.eof() )
{
// get the weapon's name and other attributes
input >> name;
input >> power >> value >> weight;

//make a Weapon object
Weapon* w = new Weapon(
}


The problem is, I have need to have access Weapon's private data member (m_Power, m_value, m_Weight). My Weapon default constructor though, doesn't have them as arguments: Weapon( int health = 100, std::string type = "IRON_SWORD" );
I don't want to change the constructor because that's what I'm going to use when I'll be creating objects later. I won't make it a friend because you said it's not a good idea..Maybe make functions in Weapon like:

void Weapon::setPower( int power )
{
m_Power = power;
}





Is that better?

Share this post


Link to post
Share on other sites
Quote:
Original post by sheep19
By the way, is there a way to restrict me so I can make only one of the above object? Like when I try to make 2 or more, I get an error, or nothing happens.
There are ways of doing it, yes, but are you sure you really want to? It might be quite nice to be able to have three separate libraries backed by three separate files. You could store all the weapons that are generic, enemy-dropped ones in one library, and all the special 'super-weapons' that you only get when you kill a boss in another library. That way when you need an enemy to drop a weapon you can just pick any one at random from the first library, without them accidentally dropping something uber-powerful.

Share this post


Link to post
Share on other sites

This topic is 3454 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this