Sign in to follow this  

Is this good OOP?

This topic is 3631 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

Hello, I'm trying to make a simple text console RPG in C++. So far I have created player and enemy class. It compiles fine btw. The header file: declarations.h
#include <iostream>
#include <string>
#include <vector>

class Player
{
public:
	// default constructor initialization
	Player (std::string name = "Fro", int healthMax = 10, int health = 10, int attack = 4, int defence = 4, int speed = 3);

	void ShowStats() const; // show the player's stats (health, attack etc)
	void Attack(); // attack an enemy
	void RunAway(); // run away
	void DecreaseHealth (int damage); //decrease the player's health when he has been attacked
private:
	std::string m_Name;
	int m_Level, m_Experience;
	int m_HealthMax, m_Health, m_Attack, m_Defence, m_Speed;
	//needs a vector of Items
};

class Enemy
{
public:
	void ShowStats() const;
	void Attack();
	void RunAway();
	void DecreaseHealth();
private:
	int m_Level;
	int m_HealthMax, m_Health, m_Attack, m_Defence, m_Speed;
};



player.cpp -> Where I put functions for player and enemy (thinking of putting enemy in another file)
#include "declarations.h"
using namespace std;

Player::Player (string name, int healthMax, int health, int attack, int defence, int speed)
: m_Name( name ),	m_HealthMax( healthMax ),	m_Health( m_HealthMax ),
  m_Attack( attack ),	m_Defence( defence ),	m_Speed( speed )
{
}

void Player::ShowStats() const
{
	cout << "Your stats:\n"
		 <<  "Name: " << m_Name << "\n"
		 << "Health: " << m_Health << " of " << m_HealthMax << "\n"
		 << "Attack: " << m_Attack << "\n"
		 << "Defence: " << m_Defence << "\n"
		 <<"Speed: " << m_Speed << "\n"
		;
}
void Player::DecreaseHealth (int damage) 
{
	m_Health -= damage;
	if (m_Health < 0)
		m_Health = 0;

	cout << "You've taken " << damage << " damage.\n"
		 << "Your health has become " << m_Health << ".\n"
		 ;
}


Am I taking a good OOP approach? Thanks in advance. [Edited by - sheep19 on January 6, 2008 3:07:21 AM]

Share this post


Link to post
Share on other sites
Constructors: use initialization list.

Don't use warts on your identifiers: the m_ prefix doesn't really communicate that much, so just drop it.

Your output is split over a whole bunch of functions in different classes. What if you decided you wanted to log each game to file as well? Then you'd have to modify each function to add in this optional behavior. Not elegant. Instead, have your ShowStats() functions return a string, then you can send that to cout (or a file, or a network socket, as appropriate) in one place.

In your DecreaseHealth() function, what happens if m_health is already zero? Oh, I know, the final health remains zero, but the function still prints out that you took X damage... when you didn't.


As for whether this is "good OOP," your program doesn't do enough - it doesn't have enough complexity - to substantially benefit. Nevertheless, you're off to a good start.

Share this post


Link to post
Share on other sites
Your Player class and Enemy class have allot of variables and functions in common. To really take advantage of OOP, you should make a base class that you perhaps could call "Creature" thats has all the standard variables that any creature of any kind in your game will have.

[source langg = "cpp"]
class Creature
{
private:
int m_HealthMax, m_Health, m_Attack, m_Defence;
Vec2d const m_Vel, m_Pos; //velocity and position vector
public:
void ShowStats() const;
void Attack(Creature* enemy); //now every creature can attack each other too
void RunAway();
void DecreaseHealth();

Creature( int healthMax = 1, int health = 1, int attack = 1, int defence = 1, int speed = 1);
};
class Player: Creature
{
string name;
Player (std::string nickname = "Fro", int healthMax = 10, int health = 10, int attack = 4, int defence = 4, int speed = 3): Creature(healthMax,health,attack,defence,speed);
{
//let the creature class take care of everything 'basic'.
name = nickname;
}
// add variables that is specifik for the Player. The player class can call
// functions declared in Creature. If something differense shall happen when
// he fx. Player->RunAway(), then rewrite that function in the Player class
}
class Enemy: Creature
{

}

class Boss: Enemy
{

}



The code isn't tested or anything
you may want to make a totally virtual Creature class.

Share this post


Link to post
Share on other sites
Along with Oluseyi's suggestions, I would add that you should try to factor out common functionality into a common base and avoid methods that do more than one function (eg. modify values and print results to the user).

I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.

As a general advice, if you're really interested in OO design then try to get your hands on the GOF (if you haven't already done so).

Share this post


Link to post
Share on other sites
Quote:
Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.


Setters and getters are best avoided. In a well written program, functions are short: making local variables obvious due to their declaration.

Share this post


Link to post
Share on other sites
I agree with MadsGustaf and Lotus that the m_VarName or mVarName are useful and do contain valuable information as to whether the variable is a local or a member variable without having to look back at the class. Many company style guides I've seen will require this in code.

I disagree that Get and Set functions are not valuable, they make it easier to refactor code at a later date. Making your member variables public is bad style and makes it difficult to modify functionality later. An example would be when the mHealth variable is set <= 0 you could later add a flag that gets set or a call to another function. If you let code outside of your class modify the variable directly you are locked out of this functionality and would have to modify code everywhere someone modified the mHealth variable.

As MadsGustaf said, group common functionality into a base class something like class Entity and derive class Player and class Enemy from it.

Share this post


Link to post
Share on other sites
Quote:
Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.


You could allways use the this keyword.

Share this post


Link to post
Share on other sites
Quote:
Original post by mwnoname
I disagree that Get and Set functions are not valuable, they make it easier to refactor code at a later date. Making your member variables public is bad style and makes it difficult to modify functionality later. An example would be when the mHealth variable is set <= 0 you could later add a flag that gets set or a call to another function. If you let code outside of your class modify the variable directly you are locked out of this functionality and would have to modify code everywhere someone modified the mHealth variable.


Note: I did not mention "prefer public variables".

If your code is like this then something is very wrong

player.setHealth( player.getHealth() - someValue );

Instead, use domain specific alternatives, such as the one the OP used "DecreaseHealth()". This makes the class an actor with meaningful actions, rather than a data holder that can only do validation. Applying this liberally and I rarely find myself needing to write set functions. I only write the equivalent get function (though I don't name it such, I would write int health() const) to allow other objects to inspect the one in question.

Share this post


Link to post
Share on other sites
Quote:
Original post by MadsGustaf
The code isn't tested or anything
you may want to make a totally virtual Creature class.


Why not make a pure virtual object class, as you may find that game items will share similar functions with monsters/players.
You can then make a player class, enemy class and item class. Then, new items can inherit from item class. I remember making an ASCII text adventure game like that.

Share this post


Link to post
Share on other sites
Quote:
Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.

If you really believe that, your functions are too long.

Quote:
As a general advice, if you're really interested in OO design then try to get your hands on the GOF (if you haven't already done so).

GoF is a patterns book, not an OO design book.

Quote:
Original post by mwnoname
I agree with MadsGustaf and Lotus that the m_VarName or mVarName are useful and do contain valuable information as to whether the variable is a local or a member variable without having to look back at the class. Many company style guides I've seen will require this in code.

Many company style guides are crap.

Quote:
I disagree that Get and Set functions are not valuable, they make it easier to refactor code at a later date.

Get* and Set* are semantic code smell; prefer action methods that localize the behavior within the class. There is virtually no reason to allow an external piece of code directly set a private member variable.

Share this post


Link to post
Share on other sites
Quote:
Original post by rothzael
Quote:
Original post by MadsGustaf
The code isn't tested or anything
you may want to make a totally virtual Creature class.

Why not make a pure virtual object class, as you may find that game items will share similar functions with monsters/players.

YAGNI.

Too much up-front "design" and architecture, to support a bunch of flexibility you have no evidence you actually need, is bad.

Share this post


Link to post
Share on other sites
Quote:
Original post by Oluseyi
Quote:
As a general advice, if you're really interested in OO design then try to get your hands on the GOF (if you haven't already done so).

GoF is a patterns book, not an OO design book.


It's definitely a patterns book! I believe that studying elegant solutions to specific problems in object-oriented software design is great way to improve one's object oriented design insight. Of course this is just "part" of the whole (there's much more to OO design), but it is an important one.

Share this post


Link to post
Share on other sites
[quote]Original post by Oluseyi
Many company style guides are crap.[quote]

That may be true, and you may disagree with them, but if you don't follow them, you won't be working very long...

Quote:
Original post by Oluseyi
Get* and Set* are semantic code smell; prefer action methods that localize the behavior within the class. There is virtually no reason to allow an external piece of code directly set a private member variable.

I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

Share this post


Link to post
Share on other sites
Quote:
Original post by Oluseyi
Quote:
Original post by Lotus
I do not think the prefix m_ is that bad though, it does help avoid having to rename setter method parameter names to avoid conflicts, as well as make the distinction between temporary and class-level variable.

If you really believe that, your functions are too long.


I disagree, it has little to do with the length of the function. When I look at a variable, I want to immediately know if it is a local or if it is a member. I shouldn't need to look a few lines up to see if it's a parameter or a local variable. I believe it is much easier to read code using that notation.

Hungarian notation was created for a reason. While all of it's useful features are no longer needed because of advances with development environments, certain parts of it are still worth adhering to.

Some development enviroments are even easier to use when adhering to the mVariableName convention. What I mean is that you can type "m" and then VisualAssist or similar programs will give you the list of member variables to choose from.

Share this post


Link to post
Share on other sites
Hungarian wartation is a completely subjective thing. Personally, I agree that it's utterly hideous and pretty much useless if you have an IDE from this millenium, but hey - I'm not the one who has to stare at your code all day [wink]


For the OP - you have made one serious mistake, and that is adding functions before you actually have functionality. Specifically, I'm thinking of Attack() and RunAway() here. You've added them to the classes, but clearly haven't really thought about what they should do.

For example, when somebody calls Attack, how do you know what to attack? What rules and processes govern the combat? Does the player (or enemy character) just attack once, or keep attacking until the target dies? etc.

Or, for RunAway, what exactly do I run away from? Where do I run to?

Until you have at least some basic answers to those questions, you haven't thought out the functionality enough to be putting code behind it.


Now, at this point, I'm sure someone is going to pop in and try to whine about the "agile" fad and how you should continually make small changes to the code until it evolves into what it should be. The problem with this approach is that it often becomes tempting to put in little stubs, like you have done, since obviously you'll need something like that later on. However, since you don't have much OO design experience, you won't yet have the instincts to recognize when it is a proper time to be putting in those stubs, and what they should look like.

The net result of doing that kind of "tweak until it works" hackery - at least for newcomers to software design - is that you make more work for yourself, because you have to go back several times and make sweeping changes to handle things you didn't think about. A little bit of foresight goes a long way.


For a small text RPG, is this going to be a disasterous problem? No, probably not; but it's worth developing good habits early, and if you're interested in learning good OO design (as apparently you are), it's certainly a lesson you want to learn the easy way [smile]

Share this post


Link to post
Share on other sites
Quote:
Original post by mwnoname
Quote:
Original post by Oluseyi
Many company style guides are crap.

That may be true, and you may disagree with them, but if you don't follow them, you won't be working very long...

Which is relevant if you're working for a company with a style guide. This is a kid asking about good OO design in abstract; appealing to popularity - and citing hypothetical corporate code style guides - for a guy who's trying to learn good practice on his own is bogus.

Quote:
I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

That's a retrieval, which is fine. You don't want your renderer setting your player object's health either...

Quote:
Original post by Silicon Munky
Hungarian notation was created for a reason. While all of it's useful features are no longer needed because of advances with development environments, certain parts of it are still worth adhering to.

Hungarian Notation was created for use in assembly language, where your register variables have no type information whatsoever. You're not using assembly language, but, in this case, C++, which at the very least has variable type information, including visibility and class membership. In truth, the m_ prefix is probably the least objectionable of common identifier warts, so whatever.

Quote:
Some development enviroments are even easier to use when adhering to the mVariableName convention. What I mean is that you can type "m" and then VisualAssist or similar programs will give you the list of member variables to choose from.

You could type an object name, a period and then see the list. Or you could trigger the code completion by typing ctrl-space (in Visual Studio). Or the same environments you're talking about would still proffer completions after any character, not just m, right?

Yeah, that's a non-starter.

Share this post


Link to post
Share on other sites
Quote:

Original post by Oluseyi
Quote:
I agree, you want to localize the behavior wherever possible, but there are many cases where you don't want to create cross dependencies between systems. For example, you probably don't want your player class rendering your health status bar...

That's a retrieval, which is fine. You don't want your renderer setting your player object's health either...


Could you explain this a bit more please? I'm also one of those people who have a tendency to (over)use get/set methods, and I'd like to better understand when they are appropriate and when they are not.

Share this post


Link to post
Share on other sites
Quote:
Original post by Gage64
Could you explain this a bit more please? I'm also one of those people who have a tendency to (over)use get/set methods, and I'd like to better understand when they are appropriate and when they are not.

It's actually fairly simple. Properties (or Get/Set methods) are intended to act as a controlled interface to internal data of a class or object. Retrieving the data (Get) generally isn't a problem, unless Get is returning a complex object that is reference bound to your instance (say something that manages an external resource like memory). To illustrate, having a getHealth() member in a CombatUnit class is unlikely to cause any problems.

On the other hand, you want to control not only what happens when a property is modified, but which pieces of code can attempt to modify the property (as much as possible), and in what ways. It's one thing to add validation to setHealth(); it's a more important thing to ask why we're allowing any code that holds a reference to a CombatUnit instance to attempt to directly set its health in the first place. In all likelihood, we'd much rather have a function takeDamage(float impactForce, float distance), for example, that computes how much the health of the CombatUnit should drop - if at all - due to a given force incident at a given distance.

This allows us to fully encapsulate the behavior of the CombatUnit within itself (any other way and you have its response to attack actually being implemented outside itself). This also allows us to upgrade how CombatUnits respond to incident force in a single place, rather than looking for all the potential bits of code that might have called setHealth().

Hope that helps.

Share this post


Link to post
Share on other sites
Quote:

Original post by Oluseyi
Hope that helps.


It certainly does! Thank you for the detailed explanation.

BTW, can you recommend a resource (book\article\...) that talks more about this sort of thing? Something beyond an introductory book on an OOP language but not as complex as a large book on OOD (I don't think I'm ready for those yet)?

Share this post


Link to post
Share on other sites
Quote:
Original post by Gage64
Quote:
Original post by Oluseyi
Hope that helps.

It certainly does! Thank you for the detailed explanation.

You're very welcome!

Quote:
BTW, can you recommend a resource (book\article\...) that talks more about this sort of thing? Something beyond an introductory book on an OOP language but not as complex as a large book on OOD (I don't think I'm ready for those yet)?

Unfortunately, I can't. I really don't read very many "technical principles" books any more. (Here's a bit of trivia: I generally don't bother to install C++ compilers anymore; for my personal work it's too slow in terms of iterating over different approaches, and I work in a .NET shop now.) I code for work and play, I discuss code and design with others, and I try out different approaches. As I get older (and my interests expand to other forms of design - industrial design, automotive design, furniture, art), I often find myself dissatisfied with existing/familiar solutions and start experimenting to find new ones.

I'm sorry I can't recommend a text, but I hope the comments about constantly challenging yourself and reviewing your familiar process are useful.

Share this post


Link to post
Share on other sites
Quote:
Original post by Oluseyi
Don't use warts on your identifiers: the m_ prefix doesn't really communicate that much, so just drop it.

I would say that the _ doesn't add anything(though that is a preference, not a style-guide)
The m tells you that it is a member. Just by looking at aNewState == mState I can see that it is comparing an argument with a member. I don't have to hover the variable.
this-> doesnt work. You could forget this-> on some member which makes it only useful for variable collisions.

Quote:
Original post by Oluseyi
Hungarian Notation was created for use in assembly language, where your register variables have no type information whatsoever.

I though Apps hungarian notation was created by Microsofts Apps division for use in word and excel. Then the rest of the world got it wrong and interpreted as System hungarian notation. Apps is somewhat useful in c++, system is not. (my (history) reference)

Quote:
Original post by Oluseyi
In all likelihood, we'd much rather have a function takeDamage(float impactForce, float distance), for example, that computes how much the health of the CombatUnit should drop - if at all - due to a given force incident at a given distance.

It is kinda interesting that the CombatUnit should be responsible for calculating the damage from the force and distance. I would rather design it with a extra step like combatUnit.takeDamage( CalculateDamage(force, distance) );

Share this post


Link to post
Share on other sites
Quote:
Original post by sirGustav
I would say that the _ doesn't add anything(though that is a preference, not a style-guide)
The m tells you that it is a member. Just by looking at aNewState == mState I can see that it is comparing an argument with a member. I don't have to hover the variable.

state = newState;

If your member functions are compact enough, you won't have forgotten that state is a member variable when reading - and even if you jump directly to that line from an error, for instance, the function will fit comfortably on a single page. (In the latter case, you'd still need to hover the variable, since the m prefix doesn't tell you what type the member is.)

Really, it's a preference issue. I think it doesn't provide any benefit; your mileage varies.

Quote:
I though Apps hungarian notation was created by Microsofts Apps division for use in word and excel. Then the rest of the world got it wrong and interpreted as System hungarian notation. Apps is somewhat useful in c++, system is not. (my (history) reference)

Nope
Quote:
Hungarian notation was designed to be language-independent, and found its first major use with the BCPL programming language. Because BCPL has no data types other than the machine word, nothing in the language itself helps a programmer remember variables' types. Hungarian notation aims to remedy this by providing the programmer with explicit knowledge of each variable's data type.

Hungarian Notation was created by Charles Simonyi, who did work in the Apps division at Microsoft. However, it isn't stated that he created it while he worked at Microsoft - and Joel's article doesn't suggest that, either. To be honest, I don't know when he created it; I know it was prior to 1988 (have you ever heard of anyone using the BCPL programming language?), but that's all I know.

Quote:
It is kinda interesting that the CombatUnit should be responsible for calculating the damage from the force and distance. I would rather design it with a extra step like combatUnit.takeDamage( CalculateDamage(force, distance) );

Sure. That's the next level in abstraction - and, basically, how physics systems work: object.update(PhysicsSystem.calculateResponse(object.getPhysicalProperties));. Of course, actual implementations are more elegant than that.

Share this post


Link to post
Share on other sites
Quote:

Original post by Oluseyi
I'm sorry I can't recommend a text, but I hope the comments about constantly challenging yourself and reviewing your familiar process are useful.


They are. I was just hoping for some guidance with that path, but it looks like it's just one of those things you have to learn from experience.

Thanks again.

Share this post


Link to post
Share on other sites
Quote:
Original post by mwnoname
I agree with MadsGustaf and Lotus that the m_VarName or mVarName are useful and do contain valuable information as to whether the variable is a local or a member variable without having to look back at the class. Many company style guides I've seen will require this in code.


Why would you have to look back at the class? The absence of a local declaration tells you it has to be a member (or a global, but you should be avoiding those... it's fine to tag globals with this justification, though). If you can't see the local declarations, you're either writing too much code per function (and should refactor), or aren't taking advantage of the fact that you don't have to put them all at the top in C++ (or C99 - and you really should take advantage of this; declaration is a pain, and the old style makes it harder to reason about initialization), or both.

Quote:
I disagree that Get and Set functions are not valuable, they make it easier to refactor code at a later date. Making your member variables public is bad style and makes it difficult to modify functionality later. An example would be when the mHealth variable is set <= 0 you could later add a flag that gets set or a call to another function. If you let code outside of your class modify the variable directly you are locked out of this functionality and would have to modify code everywhere someone modified the mHealth variable.


There's a hitch, though: while get and set functions protect you from literal "direct access/modification", they tend to encourage logical "direct access/modification", which is really almost as bad. In effect, the code lies to you: "I'm encapsulated, really!". Nonsense: the idea behind member functions is to make them *do* something. If you just want some storage of a related set of data, then go ahead and use a struct and make it all public. If the only thing you want to do is organize your data, then just organize your data; don't waste your time on boilerplate.

Of course, sometimes you change your mind about whether you "just want some storage of a related set of data" somewhere long down the road. Well, it sucks when that happens, but it's really not that common. You don't even need a "big design up front" to avoid it in the large majority of cases. I am telling you this from experience: don't worry about it :) (And anyway, in more modern languages like C# or Python, this is what properties are for.)




One more piece of advice: don't comment member functions to say what they do - it should already be (and is, in your case) obvious from the name. Use comments to indicate things that you *can't* indicate in code - for example, what the return value means. One common pattern in my own code comments is "try to (what the function name implies); return whether successful" (where the return value is a bool).

Share this post


Link to post
Share on other sites

This topic is 3631 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