Jump to content
  • Advertisement
Sign in to follow this  
Joshnathan

Please critic my coding style

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

If you intended to correct an error in the post then please contact us.

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];
     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?

Share this post


Link to post
Share on other sites
Advertisement
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 this post


Link to post
Share on other sites
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.

Share this post


Link to post
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.
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.

Share this post


Link to post
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 this post


Link to post
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?

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!