Hey,

I hope this isn't the wrong forum to post in. A search of the forum shows that most people post code review requests here, but mods, if there's a specific place for this, please relocate it :)

Anyhow, I started programming this January. I've mostly used online tutorials, and I fear I've likely picked up some bad habits. There are also probably huge gaping holes in my programming knowledge. I really wanted to post a code review to hopefully correct any of these missteps before they become too ingrained and habitual. There are probably tons of things I'm completely inadvertently doing wrong despite the code being functional.

Most of this year, I've been making little 2d games, but this is my first foray into 3D programming and OpenGL. It's a little space sim that generates about 900 solar systems with orbiting planets with atmospheric compositions, and things like that. At this point, it's mostly just a homemade engine, controls, a clickable UI and things of that nature, with very little "game" involved.

It's a lot of code to go through, and I totally don't expect anyone to go through it all. But, the Main.cpp file and the Planet.cpp file together should contain a pretty good sampling of anything I may be doing wrong. For the love of god though, don't look at the controls file :P It's the only one I haven't cleaned up yet. It's functional, but full of all my commented out ramblings, inefficiencies, etc. I know it's the first one listed, but unless you want to be dismayed at humanity, I advice against it. The header file is even still bloated with tons of unused variables :P

Again, I'm very new to this, and mostly want a heads up about serious mistakes I may be making, any advice for ways to make things more efficient, or any pointers for a direction to head in from here. I appreciate advice of any sort though.

The code is up at:https://github.com/Hobogames/GameTest

I look forward to any help or insights.

Cheers :) and thanks in advance.

Hard coding paths.
Mixed casing of function names.
Newing objects without appropriate deletes. (and you should probably be using smart pointers of some kind here to avoid this issue as well, along with handling the following issue)
Cleanup code that would probably be better off in destructors

Thanks for the quick reply! I need to head out here for a little bit, but when I get back I'll take a look at those (I already know I'm terrible with mixed casing, it's something I've been trying to work on. That, and using really generic names. I just draw a blank when naming things the first time). Every time I think I have a standard set for myself, I start confusing my casing and then it just goes downhill from there. I know there are several standards for casing. Is there a specific one you recommend that's easy to remember?

By hard coding paths, do you mean when pointing to folders or does this refer to something else? These paths should be relative (I thought). Unless I had changed that moments after you went over it (I had just logged over to windows to compile a copy there, and realized the font texture was hard coded so went to change it. I also realized my name was in it >.<).

but I have some follow up questions for sure , if you have time later, especially about the newing objects without appropriate deletes and smart pointers.

Edit*

eep! and you're right, I'm totally not deleting those pointers. For some reason I thought I had included them in my main CleanUp function, but had not (or moved or deleted them for some reason that I don't recall now).

And lastly, I thought I was using smart pointers. But, after reading a bit more about them, I was totally mistaken. But, they seem pretty awesome So, after reading up a bit, if I swap out the object creation using smart pointers, I won't need to delete them, correct, as they're automatically deleted?

Hey great work, i just briefly looked over main.cpp and planet.cpp.
And what caught my attention from the get go was functions like, SoftCollide, HardCollide, ObjClicked etc etc. Instead of sending in the object you want to operate on you are sending in the index. I would recommend sending in the object directly. So it would look something like this "void SoftCollide(Planet * planet);"

And i would also like to point out the usage of const. I understand it might be a hot topic where each person have his/her own idea of whats best.

But id like to suggest to try it out.
Use const for input parameters in functions that you know wont change.

Example "void UpdateWorld(const float deltaTime);"
For example in your planet constructor you have a int objtypes, thats a perfect case for where it should be const.
Const can be abit tricky to see the point of in the beginning. But for me, its really great. It makes the code more clean and it prevents mistakes.

ah, I see, so I'd just switch the index loop outside of the function and include the function with the specific object for each iteration? I think I had done it the other way just to keep the function nice and contained. I think my brain liked just calling the function once instead of putting it in a loop, even though I know that's silly since it will do it 900 times either way :P Out of curiosity, what is the benefit to doing it the other way. Is it for clarity, or is there another reason?

And, thanks for the tip with Const :) I use const a bit sporadically when it's needed, but I definitely see now how using variables as the default anytime I need a value is a bad habit :P

Use const for input parameters in functions that you know wont change.

Example "void UpdateWorld(const float deltaTime);"
For example in your planet constructor you have a int objtypes, thats a perfect case for where it should be const.
Const can be abit tricky to see the point of in the beginning. But for me, its really great. It makes the code more clean and it prevents mistakes.

That's not the best example though, since deltaTime is already copied by value (so const here might have some meaning to the programmer such as "there is no reason to ever modify the deltaTime variable inside UpdateWorld" and some people like to do it for this reason, while others think it's ridiculous, but it can't possibly affect the function's behaviour). There is no real consensus on this use of const.

Its real power comes from being used on parameters passed by reference (I don't mean a C++ reference here, just "not a deep copy") and a lot of people do it for the const-correctness benefit (despite it not being quite perfect) and to help guide their program's logical flow, while some prefer not to because it is fairly invasive by design (once you "const up" one function you pretty much have to make your entire codebase const-correct, and casting away const is often a slippery slope, so it can be a pain if you don't start upfront), or for other reasons. Almost everyone agrees that const-correctness is a good thing, so you should probably do it unless you have very compelling reasons not to.

There's also const member functions and some other stuff, which are a bit different but essentially work the same, etc... it's easy to work with it once you grasp the concept, and transfers easily to most other languages with const semantics.

Out of curiosity, what is the benefit to doing it the other way. Is it for clarity, or is there another reason?

Id say for clarity.
It sure will make it easier to work with in the long run.

Use const for input parameters in functions that you know wont change.

Example "void UpdateWorld(const float deltaTime);"
For example in your planet constructor you have a int objtypes, thats a perfect case for where it should be const.
Const can be abit tricky to see the point of in the beginning. But for me, its really great. It makes the code more clean and it prevents mistakes.

That's not the best example though, since deltaTime is already copied by value (so const here might have some meaning to the programmer such as "there is no reason to ever modify the deltaTime variable inside UpdateWorld" and some people like to do it for this reason, while others think it's ridiculous, but it can't possibly affect the function's behaviour). There is no real consensus on this use of const.

You are correct Bacterius, it was a bad example. Thanks for clarifying.

Well, I just peeked at your code, and I opened Controls.h, and the 1st thing I see is a function called "GetDirection()" but it doesn't seem to "Get" anything. There isn't a return value, nor any out variables. Typically, having a function named "GetBlah()" would return Blah in some way.

I'm sorry I didn't look at the rest, but I thought I would point that out.

Good luck.

Well, I just peeked at your code, and I opened Controls.h, and the 1st thing I see is a function called "GetDirection()" but it doesn't seem to "Get" anything. There isn't a return value, nor any out variables. Typically, having a function named "GetBlah()" would return Blah in some way.

I'm sorry I didn't look at the rest, but I thought I would point that out.

Good luck.

Yeah, that's why I specifically mentioned not to look at the Controls file in my original post : It's the one file I hadn't really cleaned up or optimized in any way, shape or form. It's there in all of it's ugly, horrible glory It's also the one file that's probably seen the most changes.

But you have a good point. I think I had originally named it that as, while it doesn't return anything, it changes the quaternion angle and axis. So in my head, i was "getting" them by running that function and was unaware that that standard is to only use it when returning values. Thanks for the heads up!

(once you "const up" one function you pretty much have to make your entire codebase const-correct, and casting away const is often a slippery slope, so it can be a pain if you don't start upfront),

Out of curiosity, what do I need to look out for when using const? I ask because, there are lots of variables, that while they're fixed now, they won't necessarily be down the line. With the example of objType above, it stays the same now, but later I plan on that being changed (let you terraform the planets). I guess I ask because since this is my first time writing a program of this size, all the code goes through some pretty heavy and repeated revisions. I know that the correct answer is probably that I should have everything planned out from the beginning, but alas, this is more of a learn-as-I-go project and I'm pretty much implementing new features wily-nily as I learn how to do things. But, will I just be making my life miserable if I need to change a const back to a variable or vice-versa down the line, or is it relatively straight forward?

Newing objects without appropriate deletes. (and you should probably be using smart pointers of some kind here to avoid this issue as well, along with handling the following issue)

This question will probably come across as terribly dumb, but my first attempts at implementing smart pointers didn't work out for me. I initially ran into the problem of, when I try to create a vector of unique_ptrs to planets and push back new planets into them, they all got deleted at the end of the function (it works great until the function is done) I then tried to make the vector at the beginning of the Run() function, so they wouldn't be deleted until the program closed, but this didn't work either(I forget the exact error). Am I thinking about this entirely wrong? Should I just create a "Planet Manger" class that creates smart pointers to new planets as a function? I've gone over a few tutorials, but I'm missing something fundamental here about how smart pointers work I think. How does one go about being able to reference the object after more than one iteration if the pointer is deleted at the end of the function it's created in? I understand (partially, at least, i think) how the "move" syntax works to pass in ownership to a new function (but it's then deleted on that functions completion).

In a nutshell, I'm failing to create a vector of smart pointers to an object (well, I can create it, but that's about it), and use that vector until the program is closed. I'll keep reading over tutorials and cppreference.com, but if anyone feels like pointing me in the right direction I would appreciate it(terrible pun only half intended )

edit**

nevermind, I think I have that sorted out. Creating the vector of unique_ptrs to planets/stars at the beginning of the Run() function works, I was just doing it wrong If there is a more intelligent way to handle this, I'm still all ears, but I think I have it functional at least

edit**

after this thread, It's been pointed out that I totally don't need to use pointers at all for the vector of objects. So, I'll update that tomorrow. Though, I'm glad I'm now aware of smart pointers, so thanks everyone still for pointing that out.