Back to For Beginners

Please critic my coding style

For Beginners

Started by Joshnathan January 08, 2005 05:08 PM

5 comments, last by Joshnathan 19 years, 3 months ago

Joshnathan

124

Author

January 08, 2005 05: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, &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?

-----------------------------Sismondi GamesStarted c++ in October 2004...

Mulligan

378

January 08, 2005 05: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

124

Author

January 08, 2005 05:24 PM

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

-----------------------------Sismondi GamesStarted c++ in October 2004...

nprz

692

January 08, 2005 05: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

2,466

January 08, 2005 05: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.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

CompileTime

150

January 08, 2005 05: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

124

Author

January 08, 2005 05: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?

-----------------------------Sismondi GamesStarted c++ in October 2004...

Please critic my coding style

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Please critic my coding style

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines