# Programming Techniques

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

## Recommended Posts

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; 

##### Share on other sites
int Game(Screen &screen) doesn't modify screen, so you'd better pass screen as reference to const (const Screen & screen).
Also a function should be better a verb than a noun because a function does some action.
So UdateGame may be better than Game?

##### Share on other sites
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)
- Declare local variables as late as possible
- Drop the typedef from struct declaration (just struct Screen {...}; )

##### Share on other sites
Unless you mean to change a primitive type (char, short, int, long,float and their unsigned versions) it makes no sense to pass them as a reference as they are going on the stack or passed through a register of the same size as the reference.

Also if a function doesn't change it's input variables and those are passed by reference pass them as const references so the caller knows that his data won't be changed. Don't mark primitive types as const parameters for functions as it doesn't matter the function can take a copy of them and change the values all the same.
Once you move to classes make sure that methods that aren't changing the internal state of the object are marked as const as well, this allows you to call these functions on const instances of that class.

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

##### Share on other sites
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 :/

##### Share on other sites
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?

##### Share on other sites

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

You can leave the typedef there.
But nowadays in C++ it's common to use "struct A" instead of "typedef struct A". The later is more C style.
There is difference of them in C, but not in C++.
http://stackoverflow.com/questions/612328/difference-between-struct-and-typedef-struct-in-c

##### Share on other sites
Ah, okay, thanks.

##### Share on other sites

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.

##### Share on other sites

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.

Aesthetic criticism is generally discouraged, because it is so subjective. Thus, I'm not telling Spirrwell "your functions are too long", just that "I've generally found that shorter functions make for more readable code, and readability is really nice."
As an example (I hope you don't mind, I'll remove if you do):
[source lang="cpp"]unsigned char GiveOne ()
{
if (index >= packetLength) {
throw failure("ReceiveBuffer::Give -- out of data");
}
unsigned char byte = buf[index];
++index;
return byte;
}[/source]
This function is extremely readable, all statements are at the same level of abstraction, and a developer who knows little to nothing about your system can immediately understand the logic. On the other hand:
[source lang="cpp"]
sock_type connect2CMW (cmwa_config_data const& configData) {
sock_type sockfd = cmw::getSocket(SOCK_STREAM);
#if 0
return sockfd;
}
#else
for (;;) {
#ifdef CMW_WINDOWS
int const basePort = 56791;
#else
int const basePort = 56789;
#endif
::std::string node(configData.localhost ? "127.0.0.1" : "www.webEbenezer.net");

for (int j = 0; j < 6; ++j) {
getaddrinfo_wrapper(node.c_str(), basePort + j % 2, &res);

#ifdef SYSLOG_AVAILABLE
syslog(LOG_INFO, "connected to CMW on port %d", basePort + j % 2);
#endif
return sockfd;
}
// 111 is connection refused and 113 is no route to host.
printf("connect() failed on port %d. errno is %d\n"
, basePort + j % 2, GetError());
}

#ifdef CMW_WINDOWS
Sleep(configData.sleepseconds * 1000);
#else
sleep(configData.sleepseconds);
#endif
}
#endif
}[/source]
this one has a few different levels of abstraction (low level OS code, med-low level application code?), is quite a bit longer, and performs several steps in an apparently complex operation, but I wouldn't be able to readily identify the boundaries or proper high level names for those steps without being more familiar with the domain. Again, I'm not saying it's bad, incorrect, or anything like that, just that subjectively, I am less able to grok this than I (think) I would be if you were to do something like:

[source lang="cpp"]
sock_type connect2CMW (cmwa_config_data const& configData) {
sock_type sockfd = cmw::getSocket(SOCK_STREAM);
//Not sure if you sometimes need to enable that disabled code? Still under development?
for (;;) {
::std::string node(configData.localhost ? "127.0.0.1" : "www.webEbenezer.net");
int port = GetBasePort();
for (int j = 0; j < 6; ++j, port += j%2) {

AttemptLogCMWConnection(port);
return sockfd;
}
}
SleepUntilRetry(configData);
}
void AttemptLogCMWConnection(int port) {
#ifdef SYSLOG_AVAILABLE
syslog(LOG_INFO, "connected to CMW on port %d", port);
#endif
}
void PrintCMWConnectionError(intport) {
// 111 is connection refused and 113 is no route to host.
printf("connect() failed on port %d. errno is %d\n"
, port, GetError());
}
void SleepUntilReconnect(cmwa_config_data const& configData) {
#ifdef CMW_WINDOWS
Sleep(configData.sleepseconds * 1000);
#else
sleep(configData.sleepseconds);
#endif
}
int GetBasePort() {
#ifdef CMW_WINDOWS
return 56791;
#else
return 56789;
#endif
}
[/source]
I may have mangled the logic, and again, I am not implying that this is better (unless your compiler is really good, it may be slower for instance). Hope this illustrates what I mean better though Edited by bimmy

##### Share on other sites
Hmm, the code formatting didn't work, so it looks like I've made an unmitigated mess of your code. This is troubling.
Edit: Fixed Edited by bimmy

##### Share on other sites

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.

That's a little extreme. You shouldn't break a function up into many smaller functions unless you need to. You seem to be advocating doing this for the sake of doing it.

##### Share on other sites

That's a little extreme. You shouldn't break a function up into many smaller functions unless you need to. You seem to be advocating doing this for the sake of doing it.

Sorry if it wasn't clear, my advice was given in the spirit of "if you aren't sure how to acquire the taste for function decomposition, then..." and not as a general practice. I believe there are few, if any, situations where a long, complex process with multiple levels of detail should be all thrown into a single function, but it takes some forced practice to determine when a function will benefit from a split.
So, to be clear, I am not saying that all functions must be a particular length, or that they should be split arbitrarily if no obvious distinction arises. What I am saying is that obvious distinctions are often present, and there are advantages to capitalizing on them.

Edit: Also, I don't believe "you need to" do anything with regards to styling in code, rather, some styles promote code reuse, some promote readability, some promote maintainability, etc. In my experience, function decomposition is a very beneficial style, and to only employ it only when I am compelled would be to forego its benefits altogether. Edited by bimmy

##### Share on other sites

[quote name='Spirrwell' timestamp='1341599698' post='4956420']
Again thanks, but I ask again, why would I remove the typedef from the struct?

You can leave the typedef there.
But nowadays in C++ it's common to use "struct A" instead of "typedef struct A". The later is more C style.
There is difference of them in C, but not in C++.
http://stackoverflow...def-struct-in-c
[/quote]
No. Don't use typedef there, it buys you nothing but more trouble. That answer you linked is wrong (it talks about C anyway).

You can't forward declare the type for starters.

also http://stackoverflow.com/a/1084204 Edited by Codarki

##### Share on other sites

I may have mangled the logic, and again, I am not implying that this is better (unless your compiler is really good, it may be slower for instance). Hope this illustrates what I mean better though

I like what you did with the port and have used that in this revised version.

 for (;;) { #ifdef CMW_WINDOWS int port = 56791; #else int port = 56789; #endif ::std::string node(configData.localhost ? "127.0.0.1" : "www.webEbenezer.net"); for (int j = 0; j < 6; ++j, port += j%2) { try { sock_type sockfd = connect_wrapper(node.c_str(), port); #ifdef SYSLOG_AVAILABLE syslog(LOG_INFO, "connected to CMW on port %d", port); #endif return sockfd; } catch (::std::exception const& ex) { // 111 is connection refused and 113 is no route to host. printf("%s", ex.what()); } } #ifdef CMW_WINDOWS Sleep(configData.sleepseconds * 1000); #else sleep(configData.sleepseconds); #endif } 

Probably I'll take your advice on adding a function for the sleep stuff also. The new connect_wrapper function takes care of the getaddrinfo stuff so those details aren't showing up like they were. I'm using connect_wrapper in this (middle) tier and in the front tier. After the change, the size of the middle tier binary stayed the same and the size of the front tier was reduced about 200 bytes. So that seems fine.

##### Share on other sites
You may be able to do the same thing with syslog:

void syslog_opt(int priority, const char * format, va_list ap) { #ifdef SYSLOG_AVAILABLE syslog(priority, format, ap); #endif }
I wouldn't be surprised if this were optimized out on systems without syslog being available.

##### Share on other sites

You may be able to do the same thing with syslog:

void syslog_opt(int priority, const char * format, va_list ap) { #ifdef SYSLOG_AVAILABLE syslog(priority, format, ap); #endif }
I wouldn't be surprised if this were optimized out on systems without syslog being available.

Thanks. I'm not sure if you were using a pseudo code form there or if there's another way to do it. This is the only way I know how to do it.
 void syslog_opt(int priority, const char * format, ...) { #ifdef SYSLOG_AVAILABLE va_list varg; va_start (varg, format); vsyslog(priority, format, varg); va_end (varg); #endif } 

That adds 92 bytes to the executable, but I think I'll keep it. Edited by wood_brian

##### Share on other sites

Thanks. I'm not sure if you were using a pseudo code form there or if there's another way to do it. This is the only way I know how to do it.

Let's call it pseudo code . I should have mentioned I was guessing at the syntax. I knew it was possible I just forgot how it was done.