coding improvement

[mary]

I am trying to learn how to write my code better, I find when I use functions, I have to run through tons of code just to see what is going on, as you can see from the code below, the one upside I find with using functions is I can find errors faster in valgrind (my debugger). What suggestions would you give to make my code easier to read?

Code: Select all

#include <SDL/SDL.h>
#include <SDL/SDL_image.h>
int main (int argc, char *argv[])
{	
	//CONSTANTS
	const int SCREEN_WIDTH = 640;
	const int SCREEN_HEIGHT = 480;
	const int BITS_PER_PIXEL = 32;
	const int TILE_WIDTH = 32;
	const int TILE_HEIGHT = 32;
	
	SDL_Surface *buffer = NULL;//creates the buffer
	SDL_Surface *tileSheet = NULL;//inititates tileSheet surface
	SDL_Surface *water = NULL;//inititates water surface
	SDL_Surface *desert = NULL;//inititates desert surface
	SDL_Surface *grass = NULL;//inititates grass surface
	tileSheet =IMG_Load("blank.png");//128x128 image
	water = IMG_Load("water.png");//128x128 image
	desert = IMG_Load("desert.png");//128x128 image
	grass =  IMG_Load("grass.png");//128x128 image
	
	Uint32 colorkey = SDL_MapRGB( tileSheet->format, 0xff, 0, 0xFF );
	SDL_SetColorKey( tileSheet, SDL_SRCCOLORKEY, colorkey );
	if (!water)
	return 1;
	if (!desert)
	return 1;
	if (!grass)
	return 1;
	if(!tileSheet)
	return 1;
	int counter_a = 0;
	int mouse_x = 0;
	int mouse_y = 0;
	int selectedBox[3]= {0,0,0};
	SDL_Event event; //creates an event
	bool done = false; //initializes done
	bool gKey = false;//initializes gkey
	bool grid = false;//initializes grid
	SDL_Init(SDL_INIT_VIDEO); //inititate SDL
	buffer = SDL_SetVideoMode(SCREEN_WIDTH, SCREEN_HEIGHT, BITS_PER_PIXEL, SDL_SWSURFACE); //sets the buffer to the screen
	if(!buffer)
	return 1;
	SDL_WM_SetCaption("Window", NULL); //sets the window caption

	while(!done)//while done is false
	{
	SDL_FillRect(buffer,NULL,0x000000); //fills the buffer with a black retangle
	SDL_Rect area = {(SCREEN_WIDTH -(TILE_WIDTH*4)), (SCREEN_HEIGHT -(TILE_HEIGHT*4)),(TILE_WIDTH*4),(TILE_HEIGHT*4)}; //{x,y,w,h}
	SDL_FillRect(buffer,&area,0xBBBBBB);//fills a rectangle to the buffer at area coordinates, and 0xBBBBBB colour
	Uint8 *key = SDL_GetKeyState (NULL); //allows us to get a keystate

	if( event.type == SDL_MOUSEMOTION )
	{ //Get the mouse offsets
	mouse_x = event.motion.x; 
	mouse_y = event.motion.y; //If the mouse is over the button
	}
	for (int j = 1; j < 5; j++) //for loop for the rows
	{
	for (int i = 1; i < 5; i++) //for loop for the tiles in rows
	{
	counter_a ++; //counts the loop
	SDL_Rect sheet = {(SCREEN_WIDTH -(TILE_WIDTH*5-(TILE_WIDTH*i))),(SCREEN_HEIGHT -(TILE_HEIGHT*j)),TILE_WIDTH,TILE_HEIGHT}; //sets box specifications

			if( ( mouse_x > sheet.x ) && ( mouse_x < sheet.x + sheet.w ) && ( mouse_y > sheet.y ) && ( mouse_y < sheet.y + sheet.h ) ) 
			//if the mouse is in the box
			{
				if( event.type == SDL_MOUSEBUTTONDOWN ) // if a mouse button is down
				{ 
				selectedBox[0] = counter_a; //moves the couter a variable out of loop
				selectedBox[1] = j;
				selectedBox[2] = i;
				}
			}
	}
	}

SDL_BlitSurface( tileSheet, NULL,buffer , &area ); //puts the tilesheet on the screen


 
	if (key[SDLK_ESCAPE]) //if escape was pressed
	done = true; //exits loop
	if (key[SDLK_BACKSPACE]) //if backspace was pressed
	selectedBox[0] = 0; //sets the selected box to 0
	if (key[SDLK_1])
	tileSheet = grass;
	if (key[SDLK_2])
	tileSheet = water;
	if (key[SDLK_3])
	tileSheet = desert;
	if (key[SDLK_g]) //if g key is pressed
	{
		if (gKey == false) //if gKey is false
		{
			gKey = true;//set gkey to true
			if (grid == true) //if the grid is on the screen
			{
			grid = false; //toggles the grid
			}
			else if (grid == false) //if the grid is on the screen
			{
			grid = true; //toggles the grid
			}
		}
	}
	else //if any other key is pressed, or no key is pressed
	{
	gKey = false; //set gkey to false
	}	
	if (grid == true) //if we put the grid on
	{
	   for (int i = SCREEN_WIDTH; i >=0; i-=TILE_WIDTH)
	//loop to the whole screen, incrementing by the tile width
	 {
	   SDL_Rect grid_vertical = {i, 0, 2, 	SCREEN_HEIGHT}; //{x,y,w,h}
	   SDL_FillRect(buffer,&grid_vertical,0xAAAAAA);
		//adds a vertical box at grid_vertical location
	   }

	   for (int i = SCREEN_HEIGHT; i >= 0; i-=TILE_HEIGHT)
		 //loop in the horizontal direction
	   {
	   SDL_Rect grid_horizontal = {0, i, SCREEN_WIDTH, 2}; //x,y,w,h
	   SDL_FillRect(buffer,&grid_horizontal,0xAAAAAA);//adds a horizontal box, at grid_horizontal location
	  }
	}
	if (selectedBox[0] >0)
	{
	//Selection Box
	SDL_Rect sheet_top = {(SCREEN_WIDTH -(TILE_WIDTH*5-(TILE_WIDTH*selectedBox[2]))),(SCREEN_HEIGHT -(TILE_HEIGHT*selectedBox[1])),TILE_WIDTH,2};//top
	SDL_Rect sheet_bottom = {(SCREEN_WIDTH -(TILE_WIDTH*5-(TILE_WIDTH*selectedBox[2]))),(SCREEN_HEIGHT -(TILE_HEIGHT*(selectedBox[1]-1))),TILE_WIDTH,2};//bottom
	SDL_Rect sheet_left = {(SCREEN_WIDTH -(TILE_WIDTH*5-(TILE_WIDTH*selectedBox[2]))),(SCREEN_HEIGHT -(TILE_HEIGHT*selectedBox[1])),2,TILE_HEIGHT};//left
	SDL_Rect sheet_right = {(SCREEN_WIDTH -(TILE_WIDTH*5-(TILE_WIDTH*(selectedBox[2]+1)))),(SCREEN_HEIGHT -(TILE_HEIGHT*(selectedBox[1]))),2,TILE_HEIGHT+2};//right
	SDL_FillRect(buffer,&sheet_top,0x00FF00);
	SDL_FillRect(buffer,&sheet_bottom,0x00FF00);
	SDL_FillRect(buffer,&sheet_left,0x00FF00);
	SDL_FillRect(buffer,&sheet_right,0x00FF00);
}
	SDL_Flip(buffer); //flips the buffer
		while(SDL_PollEvent(&event)) //if there is an event
		{
			if (event.type == SDL_QUIT)//if the program was xed out
			{
				done = true;//exit main loop
			}
		}	
	}
	SDL_FreeSurface(tileSheet);
	SDL_FreeSurface(grass);
	SDL_FreeSurface(water);
	SDL_FreeSurface(desert);
	SDL_Quit();//exit SDL
	return 0;
}

[Falco Girgis]

How much experience do you have with Object-Oriented C++? I can think of two radically different ways to direct my advice to you.

Breaking the code into C-style functions or Object-Orienting the hell out of it with C++.

Are you using C++, and if so are you familiar with classes?

[mary]

I am using c++, and am semi familiar with classes, I do have some trouble calling functions from different classes, lets say I have 3 classes, a, b, and c, and I want b and c to be able to control something in a, then I wouldn't know how to do it off hand, the only thing I could think of is using pointers, other than that I would say that I am alright at classes.

[Randi]

how would you do this with c style functions?

[mary]

I kept on getting a segmentation fault when I changed tileSheet by pressing 1,2, or 3. so I took away the SDL_FreeSurface() for desert, water, and grass, and it screw up anymore, is this the correct way of handling it? How do I know when to free the surface?

[lotios611]

You free the surface when you don't need it anymore.

[mary]

why do we not free the buffer? why would the program have a segmentation fault when I free those surfaces?

[lotios611]

I think you shouldn't free the buffer because it is managed by SDL and SDL might try to free the buffer after you've freed it, which would cause a segmentation fault. The segmentation fault that you're getting from freeing the surfaces means that you're trying to access those surfaces after they're freed. You might accidentally be freeing more then once, or you might be trying to use the surfaces after you free them.

[Bakkon]

I don't see you polling for events anywhere, so your mouse functionality isn't going to work.

I kept on getting a segmentation fault when I changed tileSheet by pressing 1,2, or 3. so I took away the SDL_FreeSurface() for desert, water, and grass, and it screw up anymore, is this the correct way of handling it? How do I know when to free the surface?

You're assigning tileSheet to point to a different surface, so the surface you initialize tileSheet to is being lost in memory. Just leave it a NULL pointer.

why do we not free the buffer? why would the program have a segmentation fault when I free those surfaces?

The surface created with SDL_SetVideoMode is freed by SDL_Quit and you should not manually free it.

[thejahooli]

I think that becoming comfortable with classes will greatly improve your code, so that would be a good thing to learn about.

[mary]

so how would you go about changing a surfaces picture, should I just say tilesheet = IMG_Load("water.png"); or tilesheet = IMG_Load("dessert.png"); ? that is all I am really want to do, as you can see by compiling it with random pictures (with the right names) that are 128x128. This program was built just to get some ideas on how I would go about making a map editor, if you click on a tile on the picture a green square appears around it. and if you press g a grid appears.

[mary]

I am more comfortable with classes than functions, mainly because when I see them they are in the main folder, and I find it makes the code more messy, but they are a definite time saver.

[Bakkon]

so how would you go about changing a surfaces picture, should I just say tilesheet = IMG_Load("water.png"); or tilesheet = IMG_Load("dessert.png"); ? that is all I am really want to do, as you can see by compiling it with random pictures (with the right names) that are 128x128. This program was built just to get some ideas on how I would go about making a map editor, if you click on a tile on the picture a green square appears around it. and if you press g a grid appears.

Doesn't sound like you quite understand pointers. You'd do something like this.

Code: Select all

SDL_Surface* tileSheet = NULL;
SDL_Surface* water = IMG_Load("water.png");
SDL_Surface* grass = IMG_Load("grass.png");
SDL_Surface* desert = IMG_Load("desert.png");

if(keys[SDLK_z])
     tileSheet = water;
else if(keys[SDLK_x])
     tileSheet = grass;
else if(keys[SDLK_c])
     tileSheet = desert;

tileSheet = NULL;
SDL_FreeSurface(water);
SDL_FreeSurface(grass);
SDL_FreeSurface(desert);

[captjack]

I am using c++, and am semi familiar with classes, I do have some trouble calling functions from different classes, lets say I have 3 classes, a, b, and c, and I want b and c to be able to control something in a, then I wouldn't know how to do it off hand, the only thing I could think of is using pointers, other than that I would say that I am alright at classes.

What you may be referring to is inheritance. It is generally considered bad form to monkey around with the data of an object directly. Using accessor functions to set/get values is preferable so that you can encapsulate the data (making it private).

Code: Select all

#include <iostream>
#include <string>

class Person
{
public:
	Person()
	{
		m_szName = "Nobody";
	}

	Person(const std::string name)
	{
		m_szName = name;
	}

	void set_name(const std::string name)
	{
		m_szName = name;
	}

	void display()
	{
		std::cout << "Name: " << m_szName << std::endl;
	}
private:
	std::string m_szName;
};

class Student : public Person
{
public:
	Student()
	{
		m_szMajor = "undeclared";
	}

	Student(const std::string name) : Person(name)
	{
		set_major("undeclared");
	}

	Student(const std::string name, const std::string major) : Person(name)
	{
		set_major(major);
	}

	void display()
	{
		Person::display();
		std::cout << "Major: " << m_szMajor << std::endl;
	}

	void change_major(const std::string newMajor)
	{
		set_major(newMajor);
	}
private:
	std::string m_szMajor;
	void set_major(const std::string major)
	{
		m_szMajor = major;
	}
};

int main(int argc, char* argv[])
{
	Student		mary("Mary", "Info Sys");
	Student		captjack;

	std::cout << "===> mary <===" << std::endl;
	mary.display();

	std::cout << "===> captjack <===" << std::endl;
	captjack.display();
	captjack.set_name("Capt Jack");
	captjack.change_major("Comp Sci");
	captjack.display();

	return 0;
}

In this example, Person is a general class. The Student class inherits from Person. Since a Student "is a" Person, I can inherit from the Person class and have access to some of the features offered in the Person class. (In this example, _name.)

If I want to modify a student's name, I can't just change the value of the (private) _name. I have to call the accessor function set_name. You can see that in action with the line captjack.set_name("Capt Jack");