Help to review my first harder game

Started by
4 comments, last by povilaslt2 8 years, 7 months ago

Hello, so today I just finished my first harder game and I want to experienced programmers try to review my code and say what's wrong and try to advice me what to do better.

Here's the source and game.

https://github.com/Povilas-Jablonskis/SideScroller/

Controls:

w for jump, climb up.

s for crouch, climb down.

a for walk left, climb.

d for walk right, climb.

mouse1 to jump( mouse must be higher than player's sprite )

ESACAPE to PAUSE

Objective is to reach the end sign.

Thanks for advance.

Advertisement

Nicely done for your "first harder game"

Clean enough and quite readable.

My only comment currently is in your timerFunc.

Clamping the upper limit of your delta time[color=blue](as you do) will cause slower computers to not be able to get past the very first obstacle.

Which is my case, not being able to jump high enough to reach the block to bump the key in the beginning of the scene sequence.

Also causing slow movement speeds.

I'd be happy to test again as the lower end machine.

Thanks for advice. I did that because when I minimise window and such then back to the game my always character falls off screen because delta time was + 1 :X. What do you advice to do with deltatime?

Ah, then perhaps if delta time is greater than one second (or slightly less, ie. as low as 200ms although I've gone down as low as 50ms) then don't do the update and render as a quick fix. Something like this would also help if there was a sudden heavy load on the machine so the frame time could be discarded.

Looking at it from a different angle would be to stop a timer in an inactive state and reset on resume.

Bottom line, you're not guaranteed to trigger at your set interval with glutTimerFunc. Your set function will fire only when other pending glut messages have completed, it just won't fire before your interval.

Cool project. I like your art style as well.

Timings are a very important part of the systems and may perhaps warrant extra research and development.

The common go to article would be http://gafferongames.com/game-physics/fix-your-timestep/

You might start there.

edit: I see you've double posted this in the announcement section

Agreed on what is already mentioned, so I'll leave that out of this post and focus somewhat more on the programming rather than the implementation details.

Most of the points following were before I played the game, but read on to see why. I also will not try to go too much in the details at first and look more at the overall structure. See the bottom part for a bit more on the details.

Regarding naming:

- Try to not mix different types of casing for variable and function (/method) names, i.e. camelCasing vs PascalCasing vs nouppercase. On this note, you could also try prefixing your global variables to make it easier to see where variables were declared.

- Use verbs for (all) function/method names. When you call a method, you want it to be as if you were telling the computer to do something (e.g. 'move', 'play'). Some of your methods are named as such, but for example 'timerFunc' could be changed to 'updateTimer' (or 'update', since it does not just update the timer).

On that topic: try to have the method do what the name of the method says it will do. Although the glut functions have naming conventions like 'timerFunc', it doesn't require you to name it 'timerFunc' too ;).

Motivation for doing so is also that although you now use the functions just for glut, as soon as they are also called from other sources, the context of where it was called from is no longer the same and someone who wants to do so might wonder what 'timerFunc' even does.

You can somewhat ignore the following 2 points if you simply did this to get it all into one file, but since it's your entire game and it's on github anyway:

- You have the access to classes in c++, embrace them! smile.png What I now see quite a bit is variables that are related to a specific entity/object (e.g. 'health'), but are declared next to 'gamepaused'.

A class for the player, for example, would add the flexibility of being able to (theoretically) add more players, each with their own properties. It would in this example also make clear whom's health we are speaking of (a player? an enemy?). Grasping this can improve your code a lot!

- I see you have some form of file loading in place. I would most definitely advise to use this for your shaders as well, rather than placing the entire source in the file.This has the advantages of removing 'clutter' from your c++ source files, not having to place line breaks all the timer and being able to use separate text editors with syntax highlighters for GLSL (Notepad++ happens to have an extension for this).

- Remove commented code that you really aren't using anymore. Especially if you are using source control (git), because you can always retrieve deleted parts you decide to reuse in the end. Removing this makes your code a bit more readable for not only other programmers, but also yourself!

- Don't limit yourself to putting all the drawing logic in one method like with DrawContent. It is a full blown 300 lines long as we speak. Try to see where you can split it up in certain functions (e.g. 'drawBonusMap' and 'drawMap').

- This one might be of personal preference, but try placing your local variable declarations at the point where you start needing them. This can make it easier to find out where in the method you start using them and possibly also whether they are still relevant to keep (removing unused variables). Admittedly, most compilers these days will indicate the latter themselves I think.

Some specifics:

- One of the objects is named 'zaidejas'. I'm not sure whether this refers to 'player' in your native language or refers to the name of your character. In case of the latter, I personally prefer to hide such details in code. This is rarely relevant for personal project, but names of elements in games, such as characters, can change often. You don't want to have to change these sort of names in your code as soon as something about your names of elements in your game change.

- In your DrawContent method, there are several 'continue' statements in the body of the 'else', however, this isn't necessary. When the if statement is not true, it will already skip all the contents in the if block. After that, it will simply continue the loop, exactly like you are trying to achieve with your continue statement.

I hope I'm not discouraging you by the amount of feedback I'm giving, because what you have created is very good!

Thank you so much for your feedback. I read all and will apply in my code!

This topic is closed to new replies.

Advertisement