# Please critic my coding style

This topic is 4790 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

## Recommended Posts

Hi all, I am am starting to program a 2D RPG in SDL, and I just wanted to ask everyone to critic the way I want to do this. at the moment I have 4 files, main.cpp, main.h, world.cpp and world.h(nothing in it atm). Here they are: main.cpp
#include <SDL/SDL.h>
#include <iostream>
#include "main.h"

using namespace std;

SDL_Surface *screen = SDL_SetVideoMode(800, 600, 32, SDL_HWSURFACE|SDL_DOUBLEBUF);

void InitSDL()
{
if(SDL_Init(SDL_INIT_AUDIO) < 0)
cout << "Error Initiating SDL...";
atexit(SDL_Quit);
}

int main(int argc, char *argv[])
{
InitSDL();
if(!InitWorld())
cout << "Error Initiating the world...";

DisplayWorld(screen);
bool running = true;

while(running)
{
SDL_Event event;
while(SDL_PollEvent(&event))
{
if(event.type == SDL_KEYDOWN)
{
running = false;
}
}
}
return 0;
}


main.h
#ifndef MAIN_H
#define MAIN_H

bool InitWorld();
void DisplayWorld(SDL_Surface*);

#endif


world.cpp
#include "world.h"
#include <SDL/SDL_image.h>
struct WORLD
{
SDL_Surface *image;
int value;
int x, y;
};

bool InitWorld()
{
WORLD world[40][30];

for(int i = 0; i < 40; i++)
{
for(int j = 0; j < 30; j++)
{
world[j].image = temp;
world[j].value = 0;
world[j].x = i * 20;
world[j].y = j * 20;
}
}
return true;
}

void DisplayWorld(SDL_Surface *screen)
{
WORLD world[40][30];

for(int i = 0; i < 40; i++)
{
for(int j = 0; j < 30; j++)
{
SDL_Rect rect;
rect.x = world[j].x;
rect.y = world[j].y;
SDL_BlitSurface(world[j].image, NULL, screen, &rect);
}
}
}


It does nothing more then display 80*60 squares on the screen. I was going to make the hero move by squares(using the MAP struct's x and y value), and colission detection would be done with the int value inside my WORLD struct. Anyone see something I could optimize already?

##### Share on other sites
You should probably create an application class, something like:

class CApp{public:    void InitSDL();    void DisplayWorld();private:    SDL_Surface *screen;    WORLD world[40][30];};

to avoid global variables. Also, putting WORLD world[40][30]; there would prevent the DisplayWorld function from having to recreate a gigantic local variable like that every time the function runs. The way you have it now is definitly a bottleneck in your code.

I do like how you format your code, very clean and easy to read. Definitly keep doing that.

##### Share on other sites
if I put "WORLD world[40][30]" in my class, don't I have to define my structure somewhere else too?

##### Share on other sites
Quote:
 Original post by Joshnathanif I put "WORLD world[40][30]" in my class, don't I have to define my structure somewhere else too?

Your world structure? Just make sure it is included and you will be fine.

##### Share on other sites
First off, two probable bugs:
1. This
SDL_Surface *screen = SDL_SetVideoMode(800, 600, 32, SDL_HWSURFACE|SDL_DOUBLEBUF);
could cause problems because it is executed before
    InitSDL();
in main.
SDL_Surface *screen = NULL
and
    InitSDL();    screen = SDL_SetVideoMode(800, 600, 32, SDL_HWSURFACE|SDL_DOUBLEBUF);
in main. Though I've never used SDL so I don't know for certain that it would cause a problem.

2. This seems to be insufficient error handling to me:
    if(!InitWorld())    cout << "Error Initiating the world...";
i.e. the program probably shouldn't continue trying to run.

I also encourage the use of the preincrement operator whenever it can be used in place of the postincrement operator, as especially when it comes to user-defined types or STL it is more efficient.

I personally prefer to put a space between 'if' and the open bracket. Same with 'while' etc.

Oh, and watch your tabbing/spacing in main.

##### Share on other sites
Hey Josh,

So you want me to be a critic welllll first things first.

Please stay consistent with spacing. Some places your using 2 tabs for scope and other places 1 space, clean that up.

Next you should really use some comments. At the bare minimmum have a file header and a function header.

When using pointers passed in to functions, I'd use a p prefix in front of the name. Or anywhere for that matter. i.e. *screen is now *pScreen

Also don't use magic numbers i.e. for world dimensions. Use a #define WORLDSIZE_X 40 etc... (or const int)

I would also use Mulligans suggestion. Keep main small and don't create functions in it, or have a main.h (imo)

As far as optimizing... Your not doing too much right now to optimize. Only optimize when you need to optimize.

There ya go that are my suggestions. Take them with a grain of salt as I'm a very critical person when it comes to code. I do think the guidelines I've outlined are worthwhile to consider.

##### Share on other sites
I try'd it out, but when I define my class in "main.h" and then make an instance in "main.cpp", I have a problem in "world.cpp" to define the "DisplayWorld()", because I can't make 2 instances of the same class (at least it didn't work). when I define the class in "world.h" and make the instance in "world.cpp" I get the same problem but then for "main.cpp"(no 2nd instance. Any idea how to solve that problem?