## Recommended Posts

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

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

##### Share on other sites
Kylotan    9986

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).

##### Share on other sites

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).

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?

##### Share on other sites
Kylotan    9986

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.

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

Edited by ????????? ????????

##### Share on other sites
matt77hias    431
1 hour ago, ????????? ???????? said:

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

Depends if the callee needs to own the pointer and the pointer becomes dangling.

Take a look at this answer on StackOverflow: https://stackoverflow.com/a/8706254/1731200

The answer explains when to use smart and raw pointers.

##### Share on other sites
Kylotan    9986

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.

##### Share on other sites
matt77hias    431
3 hours ago, Kylotan said:

you should dereference the shared pointer and pass that object by reference

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.

##### Share on other sites
Kylotan    9986

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.

##### Share on other sites
Styves    1792
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.

Edited by Styves

##### Share on other sites
matt77hias    431
1 minute ago, Styves said:

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.

Exactly my point: https://stackoverflow.com/a/8706254/1731200

Nothing wrong with using raw pointers.

##### Share on other sites
Kylotan    9986

I do understand the argument over not overusing shared_ptr, but in other languages like C# or Python, pretty much everything is a shared_ptr and those languages are productive to work with. As such, I'm happy recommending its general use as a starting point, with the other options being there to allow optimisation.

##### Share on other sites
Zipster    2365
10 hours ago, Kylotan said:

I do understand the argument over not overusing shared_ptr, but in other languages like C# or Python, pretty much everything is a shared_ptr and those languages are productive to work with. As such, I'm happy recommending its general use as a starting point, with the other options being there to allow optimisation.

C++ isn't designed to deal with the consequences of shared-only ownership semantics like C# and Python are. Imagine what would happen if you introduced a circular reference (either intentionally or otherwise) while taking this advice. I've seen it take days for even seasoned engineers to find the cycles, let alone someone new to the language.

There are plenty of concepts I consider safe to simplify for newcomers to C++, but dynamic memory and ownership semantics just aren't among them. If you're going to play with fire, you have to know what you're doing.

##### Share on other sites
Kylotan    9986

If you introduce a circular reference with shared_ptr, you get a memory leak. This is often harmless in small applications.

If you mishandle raw pointers, your program crashes stone dead, and if you debug it, often it's not obvious why it's crashing. This can be very discouraging to a newbie to C++.

I think it's important to understand the limitations of reference counting - which is why I did highlight the circular reference issue earlier in this thread - but I still think shared_ptr as default is a reasonable starting point.

##### Share on other sites
Zipster    2365

I look at it from the point of view of someone who has to work with code written by others on a day-to-day basis (as do many of us), and ultimately share the burden of any problems or issues that arise from it. And not just code written by other team members, but also code written by my past self. Some of the trickiest bugs I've encountered were caused by code written with an incomplete or incorrect understanding of core concepts -- ownership being among the most common. And as you've pointed out, these can be insidious bugs that hide in code for a long time before rearing their ugly heads at the worst possible times.

So perhaps I'm just being cynical, but I don't mind if someone is discouraged from learning C++. Do we really have any control over that anyway? I'd rather make sure that if they ever do reach the point of working with others, they're not adding to the team's burden, increasing technical debt, or otherwise causing problems that could have been avoided had they a better understanding of what they were doing.

## Create an account

Register a new account

• ### Similar Content

• By Poprocks
Hi everyone!
I've managed to build zlib, tinyxml2 and tmxparser with cmake-gui & visual studio 2017, but when I try building the example program for tmxparser (https://github.com/sainteos/tmxparser/tree/master/test) I'm getting numerous unresolved externals for errors, and in the warnings they all state the library machine type 'x64' conflicts with target machine type 'x86'. (some of the unresolved symbols include __imp__UnhandledExceptionFilter@4, __imp__getCurrentProcess@0, __imp__HeapAlloc@12 and the file referenced in the error log is MSVCRTD.lib along with varying object files)
Thing is, when I go back to the cmake configuration listings for all 3 of those libraries, i can't find anything to suggest i've explicitly configured anything as a 64bit build, and when I open the solutions cmake generated for each of the libraries, all of them (including the test project) have x86 as the current target.. what gives?

• I am working on a tool to auto-generate some C/C++ code by parsing a file. Is there a tool that provides an interface to write C/C++ code like this
Struct* pStruct = generator->beginStruct("Material"); { pStruct->addVariable("Buffer*", "pBuffer"); pStruct->addVariable("Texture*", "pTexture"); } generator->endStruct(pStruct); /* Output file struct Material { Buffer* pBuffer; Texture* pTexture; }; */ Now I know its quite trivial to write a tool like this but I am curious to know whether a tool already exists. Seems just like a tool generating json/xml but for C++.
Note: I won't be doing any fancy stuff like templates or inheritance. Just pure C structs.
Thank you

• Its been a while since I have done C++ and I was wondering if someone could help me understand local variables and what happens they go out of scope
So I have this method for creating a vertex shader and it returns a custom class called VertexShader. But before it returns the VertexShader it places it into a std::unsorted_map called vertexShaders
Shouldn't it live on in memory since it was placed into the vertexShaders map and only when the vertexShaders map is destroyed then the deconstructor for my VertexShader is called?  By doing vertexShaders[id] = vertexShader am I really just making a copy and then eventually another deconstructor for the VertexShader placed into the map will be called?

Also when using the method the destructor is called twice? VertexShader being returned from the method now also getting destroyed?
• By hyyou
I encapsulated Physics and Graphics with ECS successfully.
Here is a simplified version :-
Physics Rigid Body = Physic_MassAndInertia + Physic_Transform + Physic_Shape Graphic Polygon Body = Graphic_Transform + Graphic_Mesh I usually set their transform via :-
findService<Service_PhysicTransform>()->setPosition(physicEntity,somePos); findService<Service_GraphicTransform>()->setPosition(graphicEntity,somePos); It works so nice, and there is no problem in practice, because I always know its "type" (physic/graphic).
However, I notice that Physic_Transform and Graphic_Transfrom are very similar and duplicate.
For the sake of good practice and maintainability, I consider to group them into Generic_Transform.
findService<Service_Transform>()->setPosition(anyEntity,somePos); //cool However, there is a little difficulty.  The physic transformation is quite complex for a child inside a compound body.
Assume that a physic body B is a child of a compound body C.   In this case, B's transformation component currently has no meaning (by design).
If I want to set the child transformation setTransformation(B,(x,y,45deg)), my current physic engine will not set the B's transformation directly - it will set C's transformation that make B's position match (x,y,45deg).

Thus, it is not so practical to group them, except I code it like (ugly and worse performance):-
class Service_Transform{ public: void setPosition(Entity B,Vec2 pos){ bool checkIsPhysic = .... //check if B is physic if(checkIsPhysic){//physic Entity compound = .... //find C ComponentPtr<Transform> cCompound = compound.get<Transform>(); cCompound->pos=pos*someValue; //calculate some transformation for C }else{//graphic ComponentPtr<Transform> cTransform=B.get<Transform>(); cTransform->pos=pos; } } } Should I group them  into 1 type of component?
I probably should not group them because its meaning and related implementation are very different, but I feel guilty ... I may miss something.