How to organize class member variables and functions

[JiiPee]

Hi, Just making a practice project (learning graphics, sockets,
wxWidgets). Its a ping pong game: playing against an opponent via
internet. It all works as fine/intended. But I am more interested in
making it professionally structured (not one of those beginner
structured ones again :)), as am trying to learn/practice C++ more.
The code is now (as you can see below) structured so that the
wxWidget window class has everything in it (PingPongFrame). Obviously
only window-based things should really be there, isn't it? I guess all
the graphics (using SFML library) should have their own class (Graphics)
- so all drawing and things like that should be there. All the game
related things (like moving objects etc) should be in a new class: Game.
Then I have the socket programming. But its code is very small so I
guess it does not need a class- I can just use it inside Game class.

So all of this I think I understand. My main question is that should I
put all the functions to be as member functions of classes or global
functions inside corresponding class header? I know we should avoid
member functions, but what would be a good rule here which ones to make
global functions and which ones members? Its easier to make them
members... Almost all of them are using private members, so is it just
best to make them members even though then I get a lot of member
functions, like 10-15? Also as you can see there are very many member
variables as well... should I put all of them inside Game and Graphics
classes, or group them into new structures?

The header of the game (window) class (I know its a bit mess, but I am
not sure yet how to group them so did not change much. Plus I know the
names are not well done, but I will fix it later...). sf:: refers to
SFML graphics library:

enum class Player {LEFT, RIGHT};

class PingPongFrame: public wxFrame
{
public:
void drawPanelMouseClick(float x, float y, bool right=false);
// called from a child window to pass mouse msg
PingPongFrame(wxWindow* parent,wxWindowID id = -1);
virtual ~PingPongFrame();
private:
sf::Text pauseMessage;
sf::Text scoreMessage;
sf::Font font;

sf::RenderWindow m_sfmlWindow; // sfml window
sf::Texture GroundTexture;
sf::Texture BoxTexture;

sf::String m_tcp;
void runTcpServer(unsigned short port);
void runTcpClient(unsigned short port);

bool m_isServer_ = false;

sf::String m_message;

bool connected;
sf::TcpListener m_listener;
sf::TcpSocket m_socket;
void TcpListenClientMessages(unsigned short port);
void TcpServerStart(unsigned short port);
void TcpClientConnect(unsigned short port);
// paddless
sf::RectangleShape leftPaddle;
sf::RectangleShape rightPaddle;
sf::CircleShape ball;
sf::Sound ballSound;
sf::Sound ballSoundHit;
sf::SoundBuffer ballSoundBuffer;
sf::SoundBuffer ballSoundBufferHit;

std::vector<std::pair<sf::SoundBuffer, sf::Sound>> m_applauseSound;
std::vector<std::pair<sf::SoundBuffer, sf::Sound>> m_ohhSound;
void addSound(const char* file,
std::vector<std::pair<sf::SoundBuffer, sf::Sound>>& soundVec);
void playRandomSound(std::vector<std::pair<sf::SoundBuffer,
sf::Sound>>& soundVec);

float paddleSpeed = 400.f;
float ballSpeed = 120.f;
sf::Clock clock;
const float pi = 3.14159f;
int gameWidth = 800;
int gameHeight = 600;
sf::Vector2f paddleSize = sf::Vector2f(25, 100);
float ballRadius = 10.f;
float deltaTime = 0.0;
bool isPlaying = false;
float ballAngle = 0.f;
int m_scoreServer = 0, m_scoreClient = 0;

void drawField();
void startGame();
void updateSceen();
void moveBall();
void sendGameInfoToServer(int rightPaddleX, int rightPaddleY);
void sendGameInfoToClient();
bool parseMsgFromServer(const std::string& msg);
bool parseMsgFromClient(const std::string& msg);

void sendMsgToClient(const std::string& msg);
void doWin(Player winner, const char* msg);
void drawScore();

bool m_isSceenChanged = true;
bool m_serverWaitingForPositionResponce = false;

float scalX, scalY;
float sizeBoxX = 32.0;
float sizeBoxY = 32.0;
float sizeGroundX = 150.0;
float sizeGroundY = 50.0;
void initWorld();
void debugInt(int i);
void DrawPanel();
}

[Öö Tiib]

On Sunday, 23 November 2014 15:46:22 UTC+2, JiiPee wrote:
> The code is now (as you can see below) structured so that the
> wxWidget window class has everything in it (PingPongFrame). Obviously
> only window-based things should really be there, isn't it?

It is art to achieve clear program structure ... but the basics are
simple.

If to look at your function names then those almost always contain
a verb and a noun (like "DrawPanel", "InitWorld", "DrawScore",
"MoveBall", etc.).

So in your mind there are things (indicated by these nouns
like "panel", "world", "score", "ball" etc.) whose responsibilities
(and data) have been given to frame.

You may want to move the responsibilities and data of a "ball" to a
class named "ball". Then you have a class of object that has knowledge
and responsibilities to look, behave, sound and interact with other
objects in role of a ball.

> I guess all
> the graphics (using SFML library) should have their own class (Graphics)
> - so all drawing and things like that should be there. All the game
> related things (like moving objects etc) should be in a new class: Game.

"Graphics" does not feel like a class of object ... notice that it is
plural. It is more likely a group of classes. Same is with "game objects".
Same is with the "communications" (servers and clients).

[JiiPee]

On 23/11/2014 16:34, Öö Tiib wrote:
> On Sunday, 23 November 2014 15:46:22 UTC+2, JiiPee wrote:
>> The code is now (as you can see below) structured so that the
>> wxWidget window class has everything in it (PingPongFrame). Obviously
>> only window-based things should really be there, isn't it?
> It is art to achieve clear program structure ... but the basics are
> simple.
>
> If to look at your function names then those almost always contain
> a verb and a noun (like "DrawPanel", "InitWorld", "DrawScore",
> "MoveBall", etc.).
>
> So in your mind there are things (indicated by these nouns
> like "panel", "world", "score", "ball" etc.) whose responsibilities
> (and data) have been given to frame.
>
> You may want to move the responsibilities and data of a "ball" to a
> class named "ball". Then you have a class of object that has knowledge
> and responsibilities to look, behave, sound and interact with other
> objects in role of a ball.

Ok, so you suggest to create a lot of small classes. The ball does not
contain much as its a simple cirlcle really, but maybe it could also
have a class I agree.

How about the drawing context pointer (or the pointer to the window
where the drawing is done): Is it best to pass it always as a parameter
to a ball object when drawing needs to be done or ball object should
save the pointer to its member variable and use it instead? I guess its
better to pass it, right?

>
>> I guess all
>> the graphics (using SFML library) should have their own class (Graphics)
>> - so all drawing and things like that should be there. All the game
>> related things (like moving objects etc) should be in a new class: Game.
> "Graphics" does not feel like a class of object ... notice that it is
> plural. It is more likely a group of classes. Same is with "game objects".
> Same is with the "communications" (servers and clients).

Yes, Game class would contain a group of objects.

How about creating functions as a member functions or global functions
which take objects as a reference parameter?

[Öö Tiib]

On Sunday, 23 November 2014 19:25:01 UTC+2, JiiPee wrote:
> On 23/11/2014 16:34, Öö Tiib wrote:
> > On Sunday, 23 November 2014 15:46:22 UTC+2, JiiPee wrote:
> >> The code is now (as you can see below) structured so that the
> >> wxWidget window class has everything in it (PingPongFrame). Obviously
> >> only window-based things should really be there, isn't it?
> > It is art to achieve clear program structure ... but the basics are
> > simple.
> >
> > If to look at your function names then those almost always contain
> > a verb and a noun (like "DrawPanel", "InitWorld", "DrawScore",
> > "MoveBall", etc.).
> >
> > So in your mind there are things (indicated by these nouns
> > like "panel", "world", "score", "ball" etc.) whose responsibilities
> > (and data) have been given to frame.
> >
> > You may want to move the responsibilities and data of a "ball" to a
> > class named "ball". Then you have a class of object that has knowledge
> > and responsibilities to look, behave, sound and interact with other
> > objects in role of a ball.
>
> Ok, so you suggest to create a lot of small classes. The ball does not
> contain much as its a simple cirlcle really, but maybe it could also
> have a class I agree.

It is odd that you talk about your ball as "a simple circle". It
looks like most prominent object of your game from the few lines
of code you have posted. Lets see only data dedicated to it:

sf::CircleShape ball;
sf::Sound ballSound;
sf::Sound ballSoundHit;
sf::SoundBuffer ballSoundBuffer;
sf::SoundBuffer ballSoundBufferHit;

float ballSpeed;
float ballRadius;
float ballAngle;

In my programs most of the classes have less data members.

> How about the drawing context pointer (or the pointer to the window
> where the drawing is done): Is it best to pass it always as a parameter
> to a ball object when drawing needs to be done or ball object should
> save the pointer to its member variable and use it instead? I guess its
> better to pass it, right?

In general the window and ball are unrelated, each can live without other.
So these interfaces feel odd:

window.drawBall(); //so ball is something absolute or property of window?
or
ball.drawOnWindow(); //so window is something absolute or property of ball?

Same ball may be displayed on several windows or several balls may be
displayed on same window. May be not now but why to nail it down?
Therefore such interfaces feel reasonable:

window.draw(ball);
or
ball.drawOn(window);
or
draw(window, ball);

> >> I guess all
> >> the graphics (using SFML library) should have their own class (Graphics)
> >> - so all drawing and things like that should be there. All the game
> >> related things (like moving objects etc) should be in a new class: Game.
> > "Graphics" does not feel like a class of object ... notice that it is
> > plural. It is more likely a group of classes. Same is with "game objects".
> > Same is with the "communications" (servers and clients).
>
> Yes, Game class would contain a group of objects.

The "game" should not do things that "ball" should do or "paddle" should do.

> How about creating functions as a member functions or global functions
> which take objects as a reference parameter?

There are no absolute answers to that. If a function needs to access
internal details of objects then it is better to make it as member.
If function can be implemented in terms of public interface of objects,
then it is better to write as free function.

That is easy for me to say but novices are in difficulty with that
because they do not realize what belongs to what and what should
(or should not) expose what. For example ... there are zero cases I
can imagine where a frame window should know or have anything to do
with ball's radius ... yet it is property of your frame window.

[Richard]

[Please do not mail me a copy of your followup]

Start with SOLID principles of object oriented design:
<http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)>

Some of the stuff you already mentioned falls under SRP.

n...@notvalid.com spake the secret code
<Dylcw.939137$9I4.7...@fx35.am4> thusly:

>So all of this I think I understand. My main question is that should I
>put all the functions to be as member functions of classes or global
>functions inside corresponding class header? I know we should avoid
>member functions, but what would be a good rule here which ones to make
>global functions and which ones members?

The general advice is to make things members that must be members. If
something can be achieved outside a member function, then make a free
function that achieves it.

By this guideline, std::string should not have find_first_of as a member,
since the algorithm std::find_first_of already covers the cases handled
by the member functions. Convenience functions that delegate to the
algorithm instead of methods could have been written. std::string would
have far fewer methods without any loss in performance if this advice
had been followed for std::string.

Its easier to make them
>members... Almost all of them are using private members, so is it just
>best to make them members even though then I get a lot of member
>functions, like 10-15?

My advice would be first to split your monster class into a network of
cooperating classes that each follow single responsibility principle.
Once you do this, clumps of methods tend to move together and you'll
find that the number of methods on any single class goes down.

>Also as you can see there are very many member
>variables as well... should I put all of them inside Game and Graphics
>classes, or group them into new structures?

Many member variables are also a general indicator that your class has
too many responsibilities. Look for clumps of member variables and
methods that go together -- for instance a group of methods that access
a clump of member variables but none of the other variables. This is
a candidate for a new class that has a single responsibility and
encapsulates those related member variables. Don't be afraid of small
classes that represent one single well-defined responsibility. (Think
of something like a two-dimensional point class.) Identify "value
types" in your code -- things like string, Point2D, etc., that don't
exhibit polymorphism or abstract interfaces. A common example is a
Money class that represents some specific quantity of money in a
particular currency.

For games, try to separate behavior (game play) from graphics (rendering).

Some articles on refactoring for C++ that may give you inspiration:
<http://legalizeadulthood.wordpress.com/category/refactoring/>
--
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
The Computer Graphics Museum <http://computergraphicsmuseum.org>
The Terminals Wiki <http://terminals.classiccmp.org>
Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>