Is this good OOP?

Started by
69 comments, last by sheep19 16 years, 3 months ago
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]
Advertisement
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.
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.
•°*”˜˜”*°•.˜”*°•..•°*”˜.•°*”˜˜”*°•..•°*”˜˜”*°•.˜”*°•.˜”*°•. Mads .•°*”˜.•°*”˜.•°*”˜˜”*°•.˜”*°•..•°*”˜.•°*”˜.•°*”˜ ˜”*°•.˜”*°•.˜”*°•..•°*”˜I am going to live forever... or die trying!
nice, thanks. I'm going to change it tomorrow. It's too late now.
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).

Check out War to the Core the world domination space MOBA-RTS hybrid.
Join us on Discord.
Into sci-fi novels? Then check out Spectral Legends.

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.
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.
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.
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.
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.

This topic is closed to new replies.

Advertisement