Anything bad so far? :/

[Ryan Wilson]

So i'm trying to write a simple combat based text game and here is what I got so far :

#include <iostream>

using namespace std;

class CCharacter {
public:
	CCharacter () {
		player = new CPlayer;
		npc = new CNPC;
		health = 0;
		attackLevel = 0;
		name = "";
	};
	~CCharacter () {
		delete player;
		delete npc;
	};
	inline int getHealth () { return health; };
	inline int getAttackLevel () { return attackLevel; };
	inline int getDefenceLevel () { return defenceLevel; };
	void setHealth (int h) { health = h; };
	void setAttackLevel (int aL) { attackLevel = aL; };
	void setDefenceLevel (int dL) { defenceLevel = dL; };
	virtual void Attack () = 0;
private:
	int health;
	int attackLevel;
	int defenceLevel;
	string name;//Not using atm
	CCharacter *player;
	CCharacter *npc;
};


class CPlayer : public CCharacter {
public:
	void Attack () { cout << "You attack the monster with " << this->getAttackLevel() << " attack!" << endl;};
};

class CNPC : public CCharacter {
public:
	void Attack () {
		cout << "The monster attacks you with " << this->getAttackLevel() << " attack!" << endl;
	};
};

int main ()
{
	return 0;
}

I want to make this so when the player or npc attacks it takes the player object and takes player->setHealth ((player->getHealth () - npc->getAttackPower ()));

but i'm not sure how to go about doing it correctly :/

[Mats]

Why not just simply use the actual integers?

#include <iostream>

using namespace std;

class hero
{
public:
int health;
};

class monster
{
    public:
    int attack;
};

int main()
{
    hero myhero;
    myhero.health = 10;
    monster mymonster;
    mymonster.attack = 2;
    //If the monster attacks the player...
    myhero.health = myhero.health - mymonster.attack;
    cout << myhero.health;
    return 0;
}

This is what I always do. It seems so much simpler.

[Ryan Wilson]

Well because both hero and monster have similar attributes like health, strength, dead or not dead types etc, So it would be better to make them inherit another class that handles all of the variables like that and take more advantage of oop.

[Gaminic]

If all your private variables have both a get and set function, chances are you shouldn't be making them private anyway.

[Ryan Wilson]

Why?

[Gaminic]

You make variables private to make sure functions outside of the class can't read or write the value. If you then circumvent the privateness by providing public get and set functions, all you're doing is putting unnecessary strain on your keyboard.

[Mats]

//Make a base class
class base
{
define all your variables here as public:
};

//Hero inherits from base class.
class hero : public base
{
}

//And monster too.
class monster : public base
{
};

hero.myhero;
monster.mymonster;
myhero.health = 10;
mymonster.attack = 2;

int main()
{
//if monster attacks
myhero.health = myhero.health - mymonster.attack;
return 0;
}

[Athar]

Exactly why does CCharacter have instances of two of its subclasses? That makes no sense whatsoever.
Some less serious problems:
- Use the initializer list to initialize members, like this:
CCharacter () : health(0), attackLevel(0), defenceLevel(0) {}

- Drop the inline, it has no effect here. The compiler will inline your getters whether you want it or not.

- The default constructor initializes std::string to an empty string, no need to assign it again.

- Your getters aren't const-correct. When a member function does not modify the object, you should mark it as const:
int getAttackLevel() const { return attackLevel; }
This allows you to call the function for constant objects and from other const member functions.

- There's no need to write this-> every time when there are no ambiguities.
No need for a semicolon after defining a member function either.

Now, regarding your question, you should pass the character you want to attack to the Attack() function (why does it start with an uppercase letter anyway? That seems to go against your naming conventions). After all, you don't just attack, you attack something or someone.

[Gaminic]

@Athar: this-> is more often for lazying your way to a dropdown box of your members rather than ambiguity.

[Athar]

Yeah, that might be useful when you're extending or inheriting from a class you're not familiar with, but in this case...

[Ryan Wilson]

Arthar is this better? :

#include <iostream>

using namespace std;

class CCharacter {
public:
	CCharacter () : health (0), attackLevel (0), defenceLevel (0) {
	}
	~CCharacter () {
	}
	int getHealth () const { return health; }
	int getAttackLevel () const { return attackLevel; }
	int getDefenceLevel () const { return defenceLevel; }
	void increaseHealth (unsigned int iH) { health += iH; }
	void decreaseHealth (unsigned int dH) { health -= dH; }
	void setAttackLevel (int aL) { attackLevel = aL; }
	void setDefenceLevel (int dL) { defenceLevel = dL; }
	virtual void Attack () = 0;
private:
	int health;
	int attackLevel;
	int defenceLevel;
	string name;
};


class CPlayer : public CCharacter {
public:
	void Attack () {
		cout << "You attack the monster with " << getAttackLevel() << " attack!" << endl;
	}
};

class CNPC : public CCharacter {
public:
	void Attack () {
		cout << "The monster attacks you with " << getAttackLevel() << " attack!" << endl;
	}
};

int main ()
{
	return 0;
}

And can you explain this more please? "Exactly why does CCharacter have instances of two of its subclasses? That makes no sense whatsoever."

[Athar]

Yes, that looks better.
And I meant the following:

  player = new CPlayer;
  npc = new CNPC;

Why does a CCharacter object create a player and NPC instance on initialization?
This leads to infinite recursion and quick memory exhaustion - you create say, a Player object, which inherits from CCharacter, so the CCharacter constructor is called, which creates a Player object, which inherits from CCharacter, so the CCharacter constructor is called, which creates a Player object...

[Ryan Wilson]

Oh I see but I don't want to create a player object and npc object in main so where would I create it?

[Athar]

You'd create them in main() anyway, because there are few sensible alternatives.
What seems to be the problem?

[Ryan Wilson]

Well I was working with SDL and there was someone who used a characterManager class that allocated new players and deallocated the players and main is mainly to run initialization of the game first then run the main game loop etc. I mean I can create them in main but is that the most efficient way? If this doesn't make sense sorry lol some things I can get carried away with. But thanks for the wondering help your giving me, it means a lot.

[Athar]

Such a manager class can make sense, but it really depends.
Even so, that doesn't alleviate the need to create your player(s) at some point. Even if it's done by some class, you still have to tell it to create them - either that or your manager class creates them when it's being constructed.
But be that as it may, CCharacter is not your manager class, it's the base class for the objects you want to manage (you could make it the manager of objects of its own type by using static members and functions).

[Ryan Wilson]

Yes okay, but how would I go about CPlayer and CNPC to interact with each other for combat like the attack method? So when the player attack method runs to attack the npc it take the npc objects health and lowers it. Just a little explanation would be very helpful or where should I go to learn more on this thank you.

[Athar]

Like I said, you can pass the character you want to attack as a parameter.
For example:

struct CNPC : public CCharacter {
	void Attack (CCharacter& target) {
		target.decreaseHealth(getAttackLevel());
	}
};

[Ryan Wilson]

Sorry had to sign back in after my long reply.. lol

OT : Would this be okay instead of a struct? And why a struct instead of a class?

This is what I have now :

#include <iostream>

using namespace std;

class CCharacter {
public:
	CCharacter () : health (0), attackLevel (0), defenceLevel (0) {
	}
	~CCharacter () {
	}
	int getHealth () const { return health; }
	int getAttackLevel () const { return attackLevel; }
	int getDefenceLevel () const { return defenceLevel; }
	void increaseHealth (unsigned int iH) { health += iH; }
	void decreaseHealth (unsigned int dH) { health -= dH; }
	void setAttackLevel (int aL) { attackLevel = aL; }
	void setDefenceLevel (int dL) { defenceLevel = dL; }
	virtual void attack (CCharacter &target) = 0;
private:
	int health;
	int attackLevel;
	int defenceLevel;
	string name;
};


class CPlayer : public CCharacter {
public:
	CPlayer () { }
	void attack (CCharacter &target) {
		cout << "You attack the monster with " << getAttackLevel() << " attack!" << endl;
		target.decreaseHealth (getAttackLevel());
		cout << "The monster got hit by a " << getAttackLevel() << ", his health is now at : " << target.getHealth() << endl;
	}
};

class CNPC : public CCharacter {
public:
	void attack (CCharacter &target) {
		cout << "The monster attacks you with " << getAttackLevel() << " attack!" << endl;
		target.decreaseHealth (getAttackLevel());
		cout << "You got hit by a " << getAttackLevel() << ", your health is now at : " << target.getHealth() << endl;
	}
};

int main ()
{
	CPlayer player;
	CNPC npc;

	player.attack (npc);
	return 0;
}

And how would I get about setting the players attributes seprately for each object? The constructor maybe but the variables are private in CCharacter class any tips? Thank you

[Athar]

Would this be okay instead of a struct? And why a struct instead of a class?

The only difference is that with struct, all members are public by default (instead of private).
If you start with public:, then you might just as well use struct. Doesn't really matter.

The constructor maybe but the variables are private in CCharacter class any tips?

Provide a CCharacter constructor that allows you to initialize these variables, e.g.:

CCharacter (int attackLevel,int defenceLevel) : health(0), attackLevel(attackLevel),
                                                defenceLevel(defenceLevel) {}

Calling a specific base constructor in a derived class is also done in the initialization list:
CNPC() : CCharacter(30,20) {}