Well recently, I figured I'd try to change the way I've been programming. I learned how to spread out data through multiple .cXX and .h files as opposed to just using a bunch of headers. My post here isn't geared towards an API or working with something really, I just want to know if the way I'm setting things up is garbage or what. Basically I've got a small SDL\OpenGL application that displays an image on the screen. I myself was simply surprised that any of it worked as usually when I try something new, everything fails.Right now, all I'm really worried about is the way I'm including header files and the way I'm handling variables a little bit differently. One thing I'm interested in is that for certain functions that need an integer or a struct I do &nameOfVariable so I don't have to return it and I can keep as many local variables as possible.

Another thing that I'm doing is making an int Game() loop, and what I do in my int main() is return Game() at the end of it. Alright, I've talked about my major concerns, I'll put up my code. I know it's quite a bit of code, I'm just asking for a skim over it and tell me if there's any major concern that pop out at you. I'm not looking for my code to be fixed, I just want to make sure that what I am doing isn't bad. Any suggestions are greatly appreciated.

Main.cpp


#include <SDL.h>
#include <SDL_audio.h>
#include <SDL_opengl.h>
#include "Init.h"
#include "Game.h"
int main(int argc, char* argv[])
{
Screen screen;
if (Initialize(0, screen, 0, 0, 0, SDL_HWSURFACE | SDL_HWACCEL | SDL_OPENGL | SDL_FULLSCREEN) < 0)
{
printf("Error: %s\n", SDL_GetError());
return -1;
}
return Game(screen);
}


Init.h:


#ifndef SCREEN_H
#define SCREEN_H
#include "Screen.h"
#endif
int Initialize(int toggleCursor, Screen &screen, int Width, int Height, int BitsPerPixel, Uint32 Flags);


Init.cpp:


#include <SDL.h>
#include <SDL_opengl.h>
#include "Init.h"
int Initialize(int toggleCursor, Screen &screen, int Width, int Height, int BitsPerPixel, Uint32 Flags)
{
if (SDL_Init(SDL_INIT_EVERYTHING) < 0)
return -1;
atexit(SDL_Quit);
SDL_ShowCursor(toggleCursor);
if (Width == 0 || Height == 0 || BitsPerPixel == 0)
{
screen.videoInfo = (SDL_VideoInfo*)SDL_GetVideoInfo();
if (Width != 0 && Height != 0)
{
screen.Width = Width;
screen.Height = Height;
}
else
{
screen.Width = screen.videoInfo->current_w;
screen.Height = screen.videoInfo->current_h;
}
if (BitsPerPixel == 0)
{
screen.BitsPerPixel = screen.videoInfo->vfmt->BitsPerPixel;
screen.BytesPerPixel = screen.videoInfo->vfmt->BytesPerPixel;
}
else
{
screen.BitsPerPixel = BitsPerPixel;
screen.BytesPerPixel = BitsPerPixel / 8;
}
screen.Flags = Flags;
}
else
{
screen.Width = Width;
screen.Height = Height;
screen.BitsPerPixel = BitsPerPixel;
screen.Flags = Flags;
screen.BytesPerPixel = BitsPerPixel / 8;
}
if (!(SDL_SetVideoMode(screen.Width, screen.Height, screen.BitsPerPixel, screen.Flags)))
return -1;
glMatrixMode(GL_PROJECTION);
glLoadIdentity();
glOrtho(0, screen.Width, screen.Height, 0, -1, 1);
glMatrixMode(GL_MODELVIEW);
glLoadIdentity();
glClearColor(0, 0, 0, 0);
glClear(GL_COLOR_BUFFER_BIT);

return 0;
}


Game.h:


#ifndef SCREEN_H
#define SCREEN_H
#include "Screen.h"
#endif
int Game(Screen &screen);


Game.cpp:


#include <SDL.h>
#include <SDL_opengl.h>
#include "Game.h"
#include "Rendering.h"
void EventHandle(int &quit, SDL_Event &event)
{
switch (event.type)
{
case SDL_QUIT:
quit = 1;
break;
case SDL_KEYDOWN:
switch (event.key.keysym.sym)
{
case SDLK_ESCAPE:
quit = 1;
break;
default:
break;
}
break;
default:
break;
}
}
int Game(Screen &screen)
{
SDL_Event event;
int quit = 0;
GLuint IMAGE;
IMAGE = LoadTexture("Image.jpg");
if (!IMAGE)
return -1;
while (!quit)
{
Render2D(0, 0, screen.Width, screen.Height, IMAGE);
if (SDL_PollEvent(&event))
{
EventHandle(quit, event);
}
SDL_GL_SwapBuffers();
}
glDeleteTextures(1, &IMAGE);
return 0;
}


Rendering.h:


GLuint LoadTexture(const char* fileName);
void Render2D(float X, float Y, float Width, float Height, GLuint texture);


Rendering.cpp:


#include <SDL.h>
#include <SDL_image.h>
#include <SDL_opengl.h>
#include "Rendering.h"
GLuint LoadTexture(const char* fileName)
{
SDL_Surface* Image = IMG_Load(fileName);
Image = SDL_DisplayFormatAlpha(Image);
if (!Image)
return NULL;
GLuint Texture;
GLenum TextureFormat;
switch(Image->format->BytesPerPixel)
{
case 3:
if (Image->format->Rmask == 0x000000ff)
TextureFormat = GL_RGB;
else
TextureFormat = GL_BGR;
break;
case 4:
if (Image->format->Rmask == 0x000000ff)
TextureFormat = GL_RGBA;
else
TextureFormat = GL_BGRA;
break;
default:
return NULL;
break;
}
glGenTextures(1, &Texture);
glBindTexture(GL_TEXTURE_2D, Texture);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
glTexImage2D(GL_TEXTURE_2D, 0, Image->format->BytesPerPixel, Image->w, Image->h, 0, TextureFormat, GL_UNSIGNED_BYTE, Image->pixels);
SDL_FreeSurface(Image);
return Texture;
}
void Render2D(float X, float Y, float Width, float Height, GLuint texture)
{
glEnable(GL_TEXTURE_2D);
glPushMatrix();
if (texture) glBindTexture(GL_TEXTURE_2D, texture);
glBegin(GL_QUADS);
glTranslatef(X, Y, 0.0f);
if (texture) glTexCoord2i(0, 0); glVertex3f(0.0f, 0.0f, 0.0f);
if (texture) glTexCoord2i(1, 0); glVertex3f(Width, 0.0f, 0.0f);
if (texture) glTexCoord2i(1, 1); glVertex3f(Width, Height, 0.0f);
if (texture) glTexCoord2i(0, 1); glVertex3f(0.0f, Height, 0.0f);
glEnd();
glPopMatrix();
glDisable(GL_TEXTURE_2D);
}


Screen.h:


typedef struct Screen
{
unsigned int Width, Height, BitsPerPixel, BytesPerPixel;
Uint32 Flags;
SDL_VideoInfo* videoInfo;
} Screen;


I don't see anything major wrong. You are doing procedural programming, which is fine. I have only couple suggestions (nitpicking really)

- Why not change[font=monospace] [/font]void EventHandle(int &quit, SDL_Event &event) to int HandleEvent(SDL_Event &event)
- Rename Game() to RunGame()
- Try to be consistent in naming local variables (quit, IMAGE, Texture)
- Return 0 instead of NULL in LoadTexture()
- Declare local variables as late as possible
- Drop the typedef from struct declaration (just struct Screen {...}; )

Thanks for all of the advice, but I'm curious, why would I remove the typedef? Is that only required when using plain C?

There are a couple of things that I've found really improve maintainability and readability (I tend towards OO and generics, but these should apply equally to Procedural):

Decompose functions. Every function should be short (2-10 lines) and every statement should be at the same level of abstraction. This will do wonders to avoid duplication and (if you name your functions well) dramatically improve readability. If you have a long function, and you're not sure how to split it up, first just chunk it into blocks, then look at what each of those blocks does. Whenever you end up with two short functions that are similar, try to make them identical and then remove one.

Maintain invariants, preconditions, and post conditions. I am not as inflexible about this as some (I generally don't assert them, etc.) but basically, if your function claims to do something, it should either do that thing or very clearly not do that thing. If you don't like to use exceptions, this can be tricky, but if you define (even just in your head) the input domain and output range and then stick to that, you will have far fewer edge cases. A simple way to do this if you don't want to deal with exceptions is to wrap primitive data types (such as the uints that opengl uses for everything) in validating structs. The struct might take a uint in the constructor, but will signal an error if that value is not valid.

If you are interested in programming style, the book Clean Code by Robert Martin is excellent, but is definitely aimed more at OO design :/

Well right now I'm working with a procedural design. As a matter of fact I'd rather not use C++ if at all possible, right now. I've been working on ripping apart everything to see how it works myself, so I can understand it better. I don't like building off of someone else's code. I mean like if I wanted a game engine, I would rather make it myself so I know how it's supposed to work and I don't have to worry about screwing things up because a tutorial or documentation is out of date, or because I simply missed a step. It's why I'm moving closer to Assembly language and eventually creating my own OS, so I know how it works.

Eventually I'll go back to at least a semi object oriented design. I guess with all that said, I'd be more on the end of a software developer than a game developer. I can put all of the fancy math together, but I couldn't make a good 3D model or a 2D image to save my life.

Again thanks, but I ask again, why would I remove the typedef from the struct?

Ah, okay, thanks.

There are a couple of things that I've found really improve maintainability and readability (I tend towards OO and generics, but these should apply equally to Procedural):

Decompose functions. Every function should be short (2-10 lines) and every statement should be at the same level of abstraction. This will do wonders to avoid duplication and (if you name your functions well) dramatically improve readability. If you have a long function, and you're not sure how to split it up, first just chunk it into blocks, then look at what each of those blocks does. Whenever you end up with two short functions that are similar, try to make them identical and then remove one.

Ten lines seems very small. Some of my functions have gotten smaller as I've worked on them, but the average is more than ten lines. I asked for advice about the code here a few months ago and no one said anything about the functions being too long.