Back to For Beginners

Private Public

For Beginners

Started by wioneo July 21, 2010 05:25 AM

16 comments, last by Sandman 13 years, 9 months ago

wioneo

110

Author

July 21, 2010 05:25 AM

Kudos to anyone who gets the SitBC referrence.

EDIT: This is a tetris clone, just realized that I din't mention that.

Anyways, I have been having trouble making any class members private, and I am told that this is bad style. So, here are the header files of my only completed project in which I had zero private members. I would greatly appreciate if someone could explain to me which members should be private and the reasoning.

c_app.h

//#pragma once#include "c_events.h"#include "c_surface.h"#include "c_control.h"class c_app : public c_events, public c_surface{    bool app_running;//exit when falsepublic:	class c_control *p_control;    bool ini_app();//initialization	int run_app();//program loop	void update_app();//all program actions	void draw_screen();//drawing actions    void ini_cleanup();//clear memory and such on exit	void ev_exit();//actions after clicking the x	void ev_keydown(SDLKey sym, SDLMod mod, Uint16 unicode);//actions afetr key press};

c_surface.h

//#pragma once#include <SDL.h>class c_surface {public:	SDL_Surface* s_screen;	SDL_Surface* load_surface(char* File);//load an image from file	bool draw_surface(SDL_Surface* s_dest,SDL_Surface* s_src, int x, int y);//draw image	bool draw_surface(SDL_Surface* s_dest,	 SDL_Surface* s_src, int x, int y, int x2, int y2, int W, int H);//overlaod	bool set_transparency(SDL_Surface* s_dest, int R, int G, int B);//set transparent color};

c_events.h

//#pragma once#include <SDL.h>class c_events{public:	virtual void do_event(SDL_Event* Event);	//Events	virtual void ev_input_focus();	virtual void ev_input_blur();	virtual void ev_keydown(SDLKey sym, SDLMod mod, Uint16 unicode);	virtual void ev_keyup(SDLKey sym, SDLMod mod, Uint16 unicode);	virtual void ev_mouse_focus();	virtual void ev_mouse_blur();	virtual void ev_mouse_move(int mx, int my, int relx, int rely, bool left,bool right,bool middle);	virtual void ev_mouse_wheel(bool up, bool down);	virtual void ev_mouse_ldown(int mx, int my);	virtual void ev_mouse_lup(int mx, int my);	virtual void ev_mouse_rdown(int mx, int my);	virtual void ev_mouse_rup(int mx, int my);	virtual void ev_mouse_mdown(int mx, int my);	virtual void ev_mouse_mup(int mx, int my);	virtual void ev_joy_axis(Uint8 which,Uint8 axis,Sint16 value);	virtual void ev_joy_bdown(Uint8 which,Uint8 butt);	virtual void ev_joy_bup(Uint8 which,Uint8 butt);	virtual void ev_joy_hat(Uint8 which,Uint8 hat,Uint8 value);	virtual void ev_joy_ball(Uint8 which,Uint8 ball,Sint16 xrel,Sint16 yrel);	virtual void ev_minimize();	virtual void ev_restore();	virtual void ev_resize(int w,int h);	virtual void ev_expose();	virtual void ev_exit();	virtual void ev_user(Uint8 type, int code, void* data1, void* data2);};

c_control.h

//#pragma once#include "c_app.h"#include "c_piece.h"#include <sdl_gfxprimitives.h>//customization#define BOARD_WIDTH 10#define BOARD_HEIGHT 20#define LINE_WIDTH 5#define PIECE_BLOCKS 5#define BLOCK_SIZE 20#define DROP_RATE 500//colors#define C_BOARD 0x00FFFFff//light blue#define C_LINE 0xFF0000ff//red#define C_BLOCK_NORMAL 0x0000FFff//blue#define C_BLOCK_PIVOT 0xEEEE00ff//yellow#define C_BLOCK_STORED 0x00FF1Aff//green#define C_BLOCK_STORED2 0xFF6600ff//orange#define C_BLOCK_GAMEOVER 0xFFFFFFff//whiteclass c_control{public:		c_control(class c_app *curr_app);	c_app *p_app;	c_piece *p_piece;		//values	int px, py, ptype, prot;//piece location, type, and rotation	int next_px, next_py, next_ptype, next_prot;	int board_x, board_y;//board position in window	int storage[BOARD_WIDTH][BOARD_HEIGHT];//locked pieces 0=empty 1=full//list of stored pieces	int line_combo;//multiple lines cleared at once	long score;//game score	long lines_done;//total liens	bool game_over;//as it says	int paused;//-1=paused	bool show_bonus;//if true show splash	int show_lines, show_change;//splash info	unsigned long time1, time2;//fps timers	//functions	void ini_board();//start game	void clear_board();//as it says	void new_piece();//create new piece	void store_piece();//lock in a piece	void draw_board();//as it says	void draw_piece(int px,int py,int ptype,int prot);//as it says	void check_lines();//check for filled lines	void clear_line(int ly);//clear filled line	bool can_move(int px,int py,int ptype,int prot);//check if move is legal	void fast_drop_piece();//immediately drop piece	void drop_piece();//gradually drop piece	void show_score(int lines, int change);//define splash	void draw_score();//render splash};

c_piece.h

//#pragma onceclass c_piece{public:	int get_block_type(int ptype, int prot, int px, int py);//return type of block at a location in piece	int get_xoff(int ptype, int prot);//return starting coords	int get_yoff(int ptype, int prot);//return starting coords};//those get functions access arrays that I added in the .cpp file but outside //of any...thing, so does that make them have global scope? I tried declaring them in the header first, but that didn't work out.

Ok so there was *one* private memebr, but that was probably accidental in all honesty. Any pointers or suggestions on which variables/functions should be private/public would be greatly appreciated.

Also, completely unrelated, can vectors be multi-dimensional?

[Edited by - wioneo on July 21, 2010 4:47:21 PM]

Bimble Bob

476

July 21, 2010 06:26 AM

As a general rule if you want some data in a class to be modified and read by just that class make it private. If you want that class and it's derivatives to access the data make it protected. Otherwise it's public. Some people will say that no data at all should ever be made public but these people are crazy for the most part since in practical applications unless you want to spend a lot of time coming up with a design that totally compliments encapsulation you're bound to have some public data members somewhere.

In c_surface I don't see why the SDL_Surface pointer needs to be public. Your public methods should provide all the manipulation necessary for other objects to operate.

You shouldn't need to have a pointer to a c_app instance in c_control though it is hard to tell without seeing the full implementation of these classes.

That's what I can see from a glance.

It's not a bug... it's a feature!

Windryder

271

July 21, 2010 06:39 AM

Quote:Original post by Dom_152Some people will say that no data at all should ever be made public but these people are crazy for the most part since in practical applications unless you want to spend a lot of time coming up with a design that totally compliments encapsulation you're bound to have some public data members somewhere.

Well-designed OO code will not need public visibility for most members. When an application "needs" to make many fields public, it is usually a good indication of poor design choices and tight component coupling. In some cases, like you said, making certain class members public is perfectly reasonable. Examples of this might be a vector class (no real point in hiding x/y/z, because they'll need to be both read and written anyway) and pure data carrier (think C-style struct) classes with little actual logic.

zealous coding - c++ programming pitfalls

Bimble Bob

476

July 21, 2010 06:55 AM

That's why I said for the most part. Since you're only going to be making small games for the time being it's not something you really need to fret about but always have it in the back of your mind at least as you build your games and apps and maybe spend a bit of time just thinking about your class hierarchy and the structure of your app. Don't be afraid to refactor if you think you can move things around to make your system more effective.

It's not a bug... it's a feature!

pulpfist

528

July 21, 2010 09:39 AM

You are asking which variables to make private/public, but the real issue here is as the other guys mentioned to "think more object oriented", and the rest will go by itself.
Let me give a very basic example.
Say that my house is a game, and me and my family members are objects in that game.
Now, my mother wants me to do the dishes.
I, as a object, could either provide a lot of variables useful for dishwashing, like hands, legs, height, etc, or I could provide a function called do_the_dishwashing()

In the first case, my mother would have to instruct me through the whole process; use you right hand and pick up the soap, use your other hand to fill the sink with hot water, so on and so forth...

In this scenario my mother would be completely tied down through the process telling me what to do and how to do it. Not only would she be tied down, she would also need to know a lot of details about me, like what a hand is, how many of those I currently got, my height, and so on.

Now, in a more object oriented scenario I would not provide any variables to the outside world, all I provided was a function called do_the_dishwashing(), and all my mother would have to do was to call this function and be on her way to other things in the mean time. I, as a object is now responsible for washing the dishes. Nobody else should care if I do it with my feet or if I have a dish washing machine to my disposal. They don't care, and shouldn't.

Ok, that may be a silly example but I'm sure you get the picture. In the latter scenario I don't need to provide direct access to my variables (hands, feet etc) to the outside world, and just as importantly, the outside world doesn't even need to know what a hand, a foot or a dish washing machine is!

Windryder

271

July 21, 2010 09:40 AM

While not strictly related to encapsulation alone, this is good reading for anyone seeking to improve their understanding of proper OO design.

EDIT: As for the OP's actual question, here are some random thoughts in no particular order:

- A good example of what's been discussed above is the SDL surface pointer in c_surface. First of all, it seems that your class has two responsibilities: loading images and managing the primary display surface. This is in violation of the Single Responsibility Principle. This class could be refactored into three (perhaps more) separate classes: one that holds the primary surface pointers and accepts draw calls to it, one that represents an image (containing a surface pointer itself) to be drawn, and one that loads images (which in turn could be abstracted to allow loading of images from different sources). Second, I see no reason for the primary display surface to be public at all. Is it ever used outside the class? If it is, you have a design problem (which I technically already pointed out). If not, why is it public in the first place?

- c_control is a very large class, and from its name and contents a quick glance does not reveal its actual purpose. It seems that you've stuffed most of the application logic into this one class, which is another SRP violation. I also see no reason for most of the fields in the class to be public at all. Again, refactor into smaller units that do one thing - not 10.

zealous coding - c++ programming pitfalls

wioneo

110

Author

July 21, 2010 04:56 PM

Quote:Original post by Windryder
- A good example of what's been discussed above is the SDL surface pointer in c_surface. First of all, it seems that your class has two responsibilities: loading images and managing the primary display surface. This is in violation of the Single Responsibility Principle. This class could be refactored into three (perhaps more) separate classes: one that holds the primary surface pointers and accepts draw calls to it, one that represents an image (containing a surface pointer itself) to be drawn, and one that loads images (which in turn could be abstracted to allow loading of images from different sources). Second, I see no reason for the primary display surface to be public at all. Is it ever used outside the class? If it is, you have a design problem (which I technically already pointed out). If not, why is it public in the first place?

So I should have separate classes for each function? Wouldn't that just create more typing with the same end result? I suppose that I will have to read that article...

I do see how making the screen's surface private would not negatively affect anything, but what are the positive effects? Visual Studio adds a 'Public:' declaration automaticaly, so it's more a matter of making things private than making them public.

Zakwayda

2,389

July 21, 2010 05:02 PM

Quote:Visual Studio adds a 'Public:' declaration automaticaly, so it's more a matter of making things private than making them public.

Other issues aside, what VS does automatically shouldn't really matter. First of all, you don't have to use the template - you can just start with an empty text file if you prefer. Second of all, it's fairly common practice to place the public interface first and the private implementation details second, so the way that particular template is set up isn't particularly unreasonable.

(Oh, and it's public:, not Public:.)

pulpfist

528

July 21, 2010 05:40 PM

Quote:
//those get functions access arrays that I added in the .cpp file but outside
//of any...thing, so does that make them have global scope? I tried declaring them in the header first, but that didn't work out.

To my knowledge the arrays will be accessible in the translation unit they are declared from the point where they are declared. They have global scope. They also exist throughout the lifetime of the program. Therefore they are accessible in the c_piece class member functions as well as any other function defined after the array declarations.

If you want to make them globally accessible in another translation unit than c_piece you could add a declaration of them in the c_piece.h file as external, and include the c_piece.h file wherever you need access to them.

wioneo

110

Author

July 21, 2010 06:27 PM

Quote:Original post by jyk
Quote:Visual Studio adds a 'Public:' declaration automaticaly, so it's more a matter of making things private than making them public.
Other issues aside, what VS does automatically shouldn't really matter. First of all, you don't have to use the template - you can just start with an empty text file if you prefer. Second of all, it's fairly common practice to place the public interface first and the private implementation details second, so the way that particular template is set up isn't particularly unreasonable.

(Oh, and it's public:, not Public:.)

Thank you for clarifying my capitalization, and I've seen several tutorials that go back and forth between private and public declarations, but that always seemed frivolous to me. Also thank you, pulpfist, for explaining that concept to me, but this leaves the cost vs. benefit question unanswered. For that specific example of privatizing the screen surface, nothing would change other than possibly losing the ability to draw on the screen from some random function, but then what is the value of that restriction? I understand that not allowing certain chanegs to protect things is good style, but if the programmer intends to change whatever it is that they are being blocked off from wouldn't they simply find a way to do so?

Also, on direct variable setters/getters, isn't having a private variable, 'x' as well as public 'get_x(){return x;' and 'set_x(arg){x=arg;}' functions essentially the same as making 'x' public to begin with? That is except with a bit of added typing, and I don't really know much about resource handling, but would 'object.x=5;' be faster than 'object.set_x(5);' if it was looped say...987 thousand times per frame for some reason?

EDIT: Quick unrelated question, what is the significance of terminating float values with 'f' like '8.0f' and the like? Does it save memory instead of recording extra zeros?

[Edited by - wioneo on July 21, 2010 8:27:43 PM]

Private Public

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Private Public

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines