I'm a C++ newbie, and I heard a lot that the best way to learn how to code is actually code. So I tried. The project is still in progress so please don't be embarrased. At least I think it's tiny enough to be read quickly.

Here it is

https://github.com/FennecFix/Delirium/tree/master/src

It written in C++ with use of Oxygine framework. Please don't mind the commit comments, it wasn't intended to show in public.

I really want to aks you to point out any flaws of my code

There's too much code there for me to find time to review it, unfortunately. It looks good and clean for the most part. In a couple of places I think there are too many getters and setters which are probably unnecessary. I also probably wouldn't personally choose a full Model / View / Controller approach for a game, but if it works for you, continue. I would also like to see more comments explaining what the code is doing, and fewer comments that tell me nothing (e.g. "//back bottom" right above 2 lines where 'backBottom' is obvious in the code).

Thanks for your reply!

Quote

I think there are too many getters and setters which are probably unnecessary

Should I make it just public?

1 hour ago, Kylotan said:

I also probably wouldn't personally choose a full Model / View / Controller approach for a game, but if it works for you, continue.

Just felt natural for me. Anyway, can you suggest what approach would you prefer?

1 hour ago, Kylotan said:

I would also like to see more comments explaining what the code is doing, and fewer comments that tell me nothing (e.g. "//back bottom" right above 2 lines where 'backBottom' is obvious in the code).

Yeah, sorry for dumb comments.

I thought my code is kinda self-explanatory, so I didn't add much comments.

I know that code review is time consuming, so maybe I just asked about it in wrong place? Can you suggest where I should go?

Usually if an object has a lot of getters and setters it implies that the interface to it is not well designed. Usually these can be removed and replaced with methods that do a better job of explaining the intent of the change or the query being performed.

If MVC feels natural to you, keep it. It looks very 'clean', once you get used to it. It can lead to functionality being split across 3x as many files as usual, but that may be a price worth paying.

Code is almost never self-explanatory. It's usually possible for a programmer to read a whole function and, if it's written well, to understand what it does. But what if I don't have time to read the whole function? Or what if there's a hidden bug, but we can't tell it's a bug because there's no comment that explains the true intention? And what if I want to understand when a certain function will be called?

I don't know if there is anywhere where people will be willing to review large amounts of code for free. There is codereview.stackexchange.com but (a) they're not game developers, and (b) they don't usually post this much code.

A few extra thoughts while I'm here:

• I see some bi-directional dependencies - such as WorldModel creating UnitModel and holding a reference to it, but that UnitModel also holds a reference back to the WorldModel. Often this is a sign of bad design because it makes the 2 classes tightly coupled. (But sometimes the alternative is worse.) And are you sure you're not creating memory leaks with these circular references? If the reference counts never hit zero due to these mutual references, then the objects won't get freed.
• You probably don't want to rely on vector length to indicate a collision or not, as in WorldModel::update. You already have a 'check' version of the function so it's not clear why you're doing this.
• Why keep calling .get() on shared_ptrs before using the value? The whole point of them is that you can use them the same way that you'd use a normal pointer.
• There's probably a more elegant way to remove your dead projectiles from the list. Maybe use a lambda with the erase/remove idiom (see example: http://en.cppreference.com/w/cpp/algorithm/remove)
• CollisionSolverAABBxAABB has some duplicated code. You want to avoid this if possible. (i.e. Don't Repeat Yourself.)
• _faceDiretion is spelled wrongly.
• I'm not a fan of a leading underscore to denote a member. It looks a lot like the syntax reserved for compiler implementation identifiers, and it's an extra thing to type when using code completion or Intellisense. I've seen codebases with trailing underscores, or `m_` prefixes, but generally I just don't use any special markup for these. It sometimes means I have to be more creative with function arguments, but it makes the rest of the code a lot cleaner.

Quote

UnitModel also holds a reference back to the WorldModel

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit. Or if unit needs to fire a bullet, it's needed to put this bullet in the world. somehow. Just don't know how else do I do that.

1 hour ago, Kylotan said:

And are you sure you're not creating memory leaks with these circular reference

1 hour ago, Kylotan said:

Why keep calling .get() on shared_ptrs before using the value? The whole point of them is that you can use them the same way that you'd use a normal pointer.

As far as I know .get() not increasing reference count, that's why I pass raw pointers. Is it a bad practice?

15 hours ago, ????????? ???????? said:

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit.

It's often possible to fix bidirectional references by having the 'owner' (in this case, the WorldModel) pass in some sort of callback or interface so that the UnitModel can query for this. Sometimes, it's okay to have the owner implement the interface - e.g. WorldModel inherits from a new 'UnitFinder' interface), meaning the physical relationship is still bidirectional (WorldModel <--> UnitModel) but the logical relationship is different (WorldModel -> UnitModel, UnitModel -> UnitFinder).

Here's some discussion on why such bidirectional references are not perfect: https://refactoring.guru/change-bidirectional-association-to-unidirectional

If this doesn't make sense to you yet, don't worry about it for now.

15 hours ago, ????????? ???????? said:

As far as I know .get() not increasing reference count, that's why I pass raw pointers. Is it a bad practice?

There is almost no good reason for passing raw pointers around if you have smart pointers available. If you want to pass the object to be used temporarily and don't feel like a shared_ptr is necessary, you should dereference the shared pointer and pass that object by reference.

I don't think the ref-counting cost is relevant in this situation.

As for nulls, he's not checking for those anyway. Besides, if you're not deliberately doing something dangerous then it's hard to end up with a null in a shared_ptr.

23 hours ago, ????????? ???????? said:

Since WorldModel keeps vector of UnitModel, it seems to me like the only way to get a list of units in sight of one unit. Or if unit needs to fire a bullet, it's needed to put this bullet in the world. somehow. Just don't know how else do I do that.

A common way to do this is with some kind of event system/queue. For simplicity sake you could also just add a small queue to your units that the world iterates over and processes, decoupling the unit from the world entirely.

4 hours ago, matt77hias said:

It could be nullptr. Furthermore, you could cache pointers in a collection (to avoid lots of unnecessary ref count increases and thus atomic adds per frame) which is not possible for references unless you use something like the std::reference_wrapper.

3 hours ago, Kylotan said:

As for nulls, he's not checking for those anyway. Besides, if you're not deliberately doing something dangerous then it's hard to end up with a null in a shared_ptr.

Most of the time I don't see a need for shared_ptr, and I've been burned before by people misusing/overusing them. In many of those situations unique_ptr was a better choice.

I don't believe shared_ptr should even be used unless ownership needs to be shared. If not, use unique_ptr and pass around references or raw pointers if the situation calls for it.

I suppose you could use weak_ptr, but I personally find the usage there irritating. It also doesn't get around the fact that ownership of the original data can still be shared.