🔒 Locked

Please critic my coding style

Started by Joshnathan Jan 8, 2005 at 5:08 PM 5 replies 800+ views

Original Post

Jan 8, 2005 at 5:08 PM

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];
     SDL_Surface *temp = IMG_Load("map.gif");
     
     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, ▭);
             }
     }
}

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?

Mulligan

Jan 8, 2005 at 5:16 PM

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.

Joshnathan

Jan 8, 2005 at 5:24 PM

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

nprz

Jan 8, 2005 at 5:34 PM

Quote:
Original post by Joshnathan
if 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.

iMalc

Jan 8, 2005 at 5:37 PM

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.
I recommend you instead do:

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.

CompileTime

Jan 8, 2005 at 5:37 PM

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.

Joshnathan

Jan 8, 2005 at 5:38 PM

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?

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.