• Advertisement
Sign in to follow this  

Private Public

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

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 false
public:

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//white

class 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 once

class 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]

Share this post


Link to post
Share on other sites
Advertisement
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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!

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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:.)

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites
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]

Share this post


Link to post
Share on other sites
Quote:

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?

Todays optimizing compilers would probably recognize and inline the set_x function in that particular case. You can read more about inline expansion here

Share this post


Link to post
Share on other sites
Quote:
and I've seen several tutorials that go back and forth between private and public declarations, but that always seemed frivolous to me.
If you just mean that switching back and forth between public and private multiple times seems frivolous, then that's just a stylistic choice. There's no requirement to do it that way, and you can certainly just have one public section and one private section if you prefer.

If you're saying that use of public and private seems frivolous in general, then I take it you haven't yet been convinced of their usefulness :)
Quote:
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?
Part of the problem is that if you can draw to the screen from some random function, then eventually you or someone else probably will draw to the screen from some random function (most likely because it'll seem convenient at the time). If you take this approach throughout your code, you may end up with a convoluted mess whose interdependencies are difficult to keep track of. Modularity and reusability will suffer, and it'll become harder to make changes to the architecture.
Quote:
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?
Ok, how would you do it? How would you go about modifying a private member variable from outside the class?

Also, keep in mind that even in real life there are plenty of examples of safeguards that can be circumvented if desired, but nevertheless provide increased safety and security. A lot of the safeguards we put in place (both in real life and in software development) aren't intended to prevent people from doing things (which is often impossible), but rather to deter people from doing things.
Quote:
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?
Yes, many would argue that having a getter/setter pair that doesn't do anything but 'get and set' has little point, and you might as well make the variable public. Furthermore, getter/setter pairs are generally discouraged in object-oriented design anyway, as they tend to expose implementation details and make the interface less abstract.

Share this post


Link to post
Share on other sites
Quote:
Original post by jyk
Yes, many would argue that having a getter/setter pair that doesn't do anything but 'get and set' has little point, and you might as well make the variable public.


One theoretical benefit of using get/set "proxy" functions is that by hiding the implementation details of the class, we're free to change how the value is set or retrieved without breaking client code. Consider the typical example where we might some day decide to retrieve a field from a database rather than simply return a stored value.

Mostly, though, you're probably better of rethinking your design or (in specific, well-considered cases) making fields public. E.g., the vector example I discussed earlier in this thread.

Share this post


Link to post
Share on other sites
I've always been bad with stylized things without clear tangible benefits/costs to compare, so I have a feeling that I might end up with bad coding tendency for the sake of personal convenience...but I can't think of anything anyone could say to affect that so...

Are there any direct processing speed differences between private and public members? I can't think of a reason for there t be...but I can't think of a lot of things.

Also, quick unrelated question, what is the significance of terminating float values with 'f' such as '8.0f' and the like? Does it save memory instead of recording extra zeros?

Share this post


Link to post
Share on other sites
Quote:
Original post by wioneo
Are there any direct processing speed differences between private and public members? I can't think of a reason for there t be...but I can't think of a lot of things.
Nope. At the end of the day, everything is just bytes in memory...
Quote:
Also, quick unrelated question, what is the significance of terminating float values with 'f' such as '8.0f' and the like?
Floating point constants in C/C++ source code are doubles by default, and the 'f' suffix forces it to be a float. Some compilers will emit a warning if you try to assign a double constant to a float, but others will silently truncate the value.
Quote:
Does it save memory instead of recording extra zeros?
Nope. All floats take 4 bytes, and all doubles take 8 bytes (on moderately sane platforms).

Share this post


Link to post
Share on other sites
Quote:
Original post by Windryder
One theoretical benefit of using get/set "proxy" functions is that by hiding the implementation details of the class, we're free to change how the value is set or retrieved without breaking client code. Consider the typical example where we might some day decide to retrieve a field from a database rather than simply return a stored value.
Right, but notice I said a getter/setter pair that "doesn't do anything but 'get and set'". By this I meant that the getter and setter don't do anything 'extra'; no checking of values, no preserving of invariants, no converting to or from some alternate internal representation. (I realize though that I didn't make this completely clear.)
Quote:
Original post by wioneo
I've always been bad with stylized things without clear tangible benefits/costs to compare, so I have a feeling that I might end up with bad coding tendency for the sake of personal convenience...but I can't think of anything anyone could say to affect that so...
Well, nobody's saying you have to write object-oriented code. If you already know you prefer a procedural approach (or a mix of procedural and object-oriented techniques), just go with it.

That said, I think it's worth considering what other, perhaps more experienced coders have to say (and here I'm not talking about myself, but rather about experts in the field and the many experienced coders who frequent this forum).

The benefits of the practices under discussion here won't necessarily be evident immediately, nor will they necessarily be evident in the context of small, one-person projects. As such, if you determine that you'll only adopt practices that have immediate, tangible benefits in the context of whatever project you're working on at the moment, you might, as you say, end up developing some bad habits as you progress.

But as I said, you should probably just go with whatever makes the most sense to you. As you write more code and as your projects become more complex, you may start to see how some of the object-oriented techniques under discussion here can be beneficial, and if so, you can always incorporate them at that time.

Share this post


Link to post
Share on other sites
Quote:
Original post by wioneo
I've always been bad with stylized things without clear tangible benefits/costs to compare, so I have a feeling that I might end up with bad coding tendency for the sake of personal convenience...but I can't think of anything anyone could say to affect that so...


In the context of your current project, there probably isn't much tangible benefit, besides getting into the habit of developing good OO coding practices.

However, as the scope of the project increases, and you (or even more importantly, other programmers) start needing to make more changes during development, the advantages will become much more apparent.

Consider the following features, and the difficulty of modifying your code to support them.

1) Variable board size selectable by the user at runtime.

2) Port to a platform on which SDL is unavailable.

3) Support for multiplayer on the same machine.

4) Support for multiplayer over a network.

5) Support for an alternative game type similar to Super Foul Egg. This should be selectable at runtime.

Some of these features would require a fairly significant amount of work anyway; but with well encapsulated modular code, they could be written (by another programmer even), and simply plugged into the code with minimal changes to the other files in the project.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement