Sign in to follow this  
MichaelBarth

Programming Techniques

Recommended Posts

MichaelBarth    341
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

[CODE]
#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);
}
[/CODE]

Init.h:

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

Init.cpp:

[CODE]
#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;
}
[/CODE]

Game.h:

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

Game.cpp:

[CODE]
#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;
}
[/CODE]

Rendering.h:

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

Rendering.cpp:

[CODE]
#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);
}
[/CODE]

Screen.h:

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

Share this post


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


Link to post
Share on other sites
Ftn    462
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 {...}; )

Share this post


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


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


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


Link to post
Share on other sites
wqking    761
[quote name='Spirrwell' timestamp='1341599698' post='4956420']
Again thanks, but I ask again, why would I remove the typedef from the struct?
[/quote]
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 this post


Link to post
Share on other sites
wood_brian    193
[quote name='bimmy' timestamp='1341597811' post='4956403']
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.
[/quote]

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 [url="http://webEbenezer.net/build_integration.html"]the code here[/url] a few months ago and no one said anything about the functions being too long.

Share this post


Link to post
Share on other sites
tim_shea    461
[quote name='wood_brian' timestamp='1341699104' post='4956753']
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.
[/quote]
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
sockaddr_in SockAddr;
SockAddr.sin_family = AF_INET;
SockAddr.sin_port = htons(56791);
SockAddr.sin_addr.s_addr = inet_addr("10.0.0.184");
if (::connect(sockfd, (sockaddr*)(&SockAddr), sizeof(SockAddr)) == 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) {
addrinfo *res;
getaddrinfo_wrapper(node.c_str(), basePort + j % 2, &res);

if (::connect(sockfd, res->ai_addr, res->ai_addrlen) == 0) {
freeaddrinfo(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());
freeaddrinfo(res);
}

#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) {
addrinfo *res;
getaddrinfo_wrapper(node.c_str(), port, &res);

if (::connect(sockfd, res->ai_addr, res->ai_addrlen) == 0) {
freeaddrinfo(res);
AttemptLogCMWConnection(port);
return sockfd;
}
freeaddrinfo(res);
}
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 [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img] Edited by bimmy

Share this post


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


Link to post
Share on other sites
sss_abaker    132
[quote name='bimmy' timestamp='1341597811' post='4956403']

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.

[/quote]
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 this post


Link to post
Share on other sites
tim_shea    461
[quote name='sss_abaker' timestamp='1341865633' post='4957399']
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.
[/quote]
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 [i]must[/i] 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 this post


Link to post
Share on other sites
Ftn    462
[quote name='wqking' timestamp='1341632878' post='4956572']
[quote name='Spirrwell' timestamp='1341599698' post='4956420']
Again thanks, but I ask again, why would I remove the typedef from the struct?
[/quote]
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++.
[url="http://stackoverflow.com/questions/612328/difference-between-struct-and-typedef-struct-in-c"]http://stackoverflow...def-struct-in-c[/url]
[/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 [url="http://stackoverflow.com/a/1084204"]http://stackoverflow.com/a/1084204[/url] Edited by Codarki

Share this post


Link to post
Share on other sites
wood_brian    193
[quote name='bimmy' timestamp='1341858346' post='4957356']
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 [img]http://public.gamedev.net//public/style_emoticons/default/smile.png[/img]
[/quote]

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

[code]
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
}
[/code]


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


Link to post
Share on other sites
nobodynews    3126
You may be able to do the same thing with syslog:

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

Share this post


Link to post
Share on other sites
wood_brian    193
[quote name='nobodynews' timestamp='1342055543' post='4958223']
You may be able to do the same thing with syslog:

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

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.
[code]
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
}
[/code]

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

Share this post


Link to post
Share on other sites
nobodynews    3126
[quote name='wood_brian' timestamp='1342062538' post='4958241']
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.
[/quote]
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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this