Sign in to follow this  
xx6heartless6xx

Shifting Away From Global Variables [C++]

Recommended Posts

I used to use many global variables and objects so many different files can all access the same variables BUT i have just learned I should avoid global variables.

Now I declared all the variables other files would need in main and pass them with functions. Is that a good way to do this? Now I have a lot of variables declared in main and is it a good idea to have a lot of variables in your main?

Also some of my objects now have 10+ variables being passed. Is this too much?

Share this post


Link to post
Share on other sites
oler1s    585
Beginners typically have a huge mass of globals because they don't have good modularity and dataflow in their programs. Because there's no clear separation of concerns in the various parts of your code, then you just have a giant pool of data that any function can use.

If you just have that entire mass in main, and then pass them all around, you haven't fixed the problem. In fact, it might just preferable to have globals, because the code will be clearer. What you really want to do is fix the underlying design. It's a bit easier to give criticism if you post actual code.

Share this post


Link to post
Share on other sites
[code]
int main( int argc, char* args[] ) {
//Level size
const int LEVEL_WIDTH = 1280; //1280
const int LEVEL_HEIGHT = 960; //960

//Screen size
const int SCREEN_WIDTH = 680; //680
const int SCREEN_HEIGHT = 420; //420

//Sprite size
const int CHARACTER_WIDTH = 32;
const int CHARACTER_HEIGHT = 32;

//Camera
SDL_Rect camera = { 0, 0, SCREEN_WIDTH, SCREEN_HEIGHT };

//Frames per sec
const int FRAMES_PER_SECOND = 20;

//Tilesheet consts
const int TILE_WIDTH = 32;
const int TILE_HEIGHT = 32;
const int TOTAL_TILES = 1200;
const int TILE_SPRITES = 529;

//Sprite from the tile sheet
SDL_Rect clips[ TILE_SPRITES ];

//Direction of character animation
const int CHARACTER_UP = 0;
const int CHARACTER_DOWN = 1;
const int CHARACTER_LEFT = 2;
const int CHARACTER_RIGHT = 3;

//Rect for character display
SDL_Rect clips_Up[ 4 ];
SDL_Rect clips_Down[ 4 ];
SDL_Rect clips_Left[ 4 ];
SDL_Rect clips_Right[ 4 ];

//Screen surface
SDL_Surface *screen;
SDL_Surface *character;
SDL_Surface *tileSheet;

//Game states
enum e_GameStates {
STATE_NULL,
STATE_LEVEL_ONE_OUTSIDE,
STATE_PAUSE,
STATE_EXIT,
};

enum e_Level {
LEVEL_ONE_OUTSIDE
};

//State variables
int stateID = STATE_NULL;
int nextState = STATE_NULL;

//Load SDL files
if( init( screen ) == false )
return 1;

//Timer Object
Timer fps;

//Event
SDL_Event event;

//Character object
Character* Mover = new Character( screen, character, event, camera, clips_Up, clips_Down, clips_Left, clips_Right, CHARACTER_WIDTH,
CHARACTER_HEIGHT, LEVEL_WIDTH, LEVEL_HEIGHT, CHARACTER_UP, CHARACTER_DOWN, CHARACTER_LEFT, CHARACTER_RIGHT );

//Tile object
Tile* tiles[ 529 ];

//Gamestate object
GameState* currentState = NULL;

//Current gamestate
stateID = STATE_LEVEL_ONE_OUTSIDE;

//Set the current gamestate object
currentState = new Level_One( tiles, Mover, event, character, tileSheet );

}
[/code]

Here are my variables I declare in my main program that are used by different classes and files. As you can see it is pretty cluttered. Also my Character class needs a lot of these variables declared in main and are being passed in the constructor.

Share this post


Link to post
Share on other sites
Toadhead    244
[quote name='xx6heartless6xx' timestamp='1310190533' post='4833002']
[code]
int main( int argc, char* args[] ) {
//Level size
const int LEVEL_WIDTH = 1280; //1280
const int LEVEL_HEIGHT = 960; //960

//Screen size
const int SCREEN_WIDTH = 680; //680
const int SCREEN_HEIGHT = 420; //420

//Sprite size
const int CHARACTER_WIDTH = 32;
const int CHARACTER_HEIGHT = 32;

//Camera
SDL_Rect camera = { 0, 0, SCREEN_WIDTH, SCREEN_HEIGHT };

//Frames per sec
const int FRAMES_PER_SECOND = 20;

//Tilesheet consts
const int TILE_WIDTH = 32;
const int TILE_HEIGHT = 32;
const int TOTAL_TILES = 1200;
const int TILE_SPRITES = 529;

//Sprite from the tile sheet
SDL_Rect clips[ TILE_SPRITES ];

//Direction of character animation
const int CHARACTER_UP = 0;
const int CHARACTER_DOWN = 1;
const int CHARACTER_LEFT = 2;
const int CHARACTER_RIGHT = 3;

//Rect for character display
SDL_Rect clips_Up[ 4 ];
SDL_Rect clips_Down[ 4 ];
SDL_Rect clips_Left[ 4 ];
SDL_Rect clips_Right[ 4 ];

//Screen surface
SDL_Surface *screen;
SDL_Surface *character;
SDL_Surface *tileSheet;

//Game states
enum e_GameStates {
STATE_NULL,
STATE_LEVEL_ONE_OUTSIDE,
STATE_PAUSE,
STATE_EXIT,
};

enum e_Level {
LEVEL_ONE_OUTSIDE
};

//State variables
int stateID = STATE_NULL;
int nextState = STATE_NULL;

//Load SDL files
if( init( screen ) == false )
return 1;

//Timer Object
Timer fps;

//Event
SDL_Event event;

//Character object
Character* Mover = new Character( screen, character, event, camera, clips_Up, clips_Down, clips_Left, clips_Right, CHARACTER_WIDTH,
CHARACTER_HEIGHT, LEVEL_WIDTH, LEVEL_HEIGHT, CHARACTER_UP, CHARACTER_DOWN, CHARACTER_LEFT, CHARACTER_RIGHT );

//Tile object
Tile* tiles[ 529 ];

//Gamestate object
GameState* currentState = NULL;

//Current gamestate
stateID = STATE_LEVEL_ONE_OUTSIDE;

//Set the current gamestate object
currentState = new Level_One( tiles, Mover, event, character, tileSheet );

}
[/code]

Here are my variables I declare in my main program that are used by different classes and files. As you can see it is pretty cluttered. Also my Character class needs a lot of these variables declared in main and are being passed in the constructor.
[/quote]

Why don't you just store the character constants and data inside the character class? Just assign those values in the class' constructor.
Store all window related data in a graphics or window class and all gamestate related code in a gamestate class.
That way all code is right where it belongs and it makes the main function much clearer.
If you need the graphics/window class to somehow interact with the character class you could add functions for communication or pass the single class as a parameter.

Share this post


Link to post
Share on other sites
jgarnett    100
If it were me I would take all of those variables (and any strings that you have) and put them in a properties file (a text file that your application loads in on start up). That way if you want to change one of them, all you need to do is modify a text file and therefore you do not need to recompile your entire program.

Share this post


Link to post
Share on other sites
iMalc    2466
So why is the tiles array exactly 529 items? Seems like a strange number. Should that actually be a compile-time calculation rather than a hard coded number? The [color="#660066"]Level_One [/color]object doesn't seem to be passed the size of this array, so it must know it somehow. This suggests that you do need a constant for that. Of course another option might be to change it to a vector anyway, which means it's no longer fixed-size, and the size is carried around with it.

Most of what I'm seing here is constants, not variables. I would treat constants more like definitions of classes in that it's perfectly okay for them to be defined in a header file somewhere. You wouldn't try and squeeze all your class defintions into main, and the same should apply to constants. Looks like some of those should be an enum actually.

Share this post


Link to post
Share on other sites
rip-off    10976
Constants aren't generally considered globals, so don't worry about them. It is mutable global values that you should avoid.

Your character class takes a lot of odd parameters. Why does it need the screen and camera? Or an event? Can the clipping rectangles be member variables?

Share this post


Link to post
Share on other sites

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