????????? ????????

C++ Code Review? Please?

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

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

Share this post


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


Link to post
Share on other sites

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?

Share this post


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


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


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


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


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


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


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


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


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


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


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


Link to post
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.

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


  • 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?
      Thanks in advance!
    • By mark_braga
      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
    • By noodleBowl
      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
      //Declared at the ShaderModule class level. Map to store VertexShaders in std::unordered_map<LPCSTR, VertexShader> vertexShaders; //Method to create a vertex shader. Returns a VertexShader VertexShader ShaderModule::createVertexShader(LPCWSTR fileName, LPCSTR id, LPCSTR entryPoint, LPCSTR targetProfile) { ID3DBlob *shaderCode = compileShaderFromFile(fileName, entryPoint, targetProfile); D3D11_INPUT_ELEMENT_DESC inputElementDescription[] = { { "POSITION", 0, DXGI_FORMAT_R32G32B32_FLOAT, 0, 0, D3D11_INPUT_PER_VERTEX_DATA, 0 }, { "COLOR", 0, DXGI_FORMAT_R32G32B32_FLOAT, 0, D3D11_APPEND_ALIGNED_ELEMENT, D3D11_INPUT_PER_VERTEX_DATA, 0 }, }; VertexShader vertexShader(id); //Create a new VertexShader device->CreateVertexShader(shaderCode->GetBufferPointer(), shaderCode->GetBufferSize(), NULL, &vertexShader.shader); device->CreateInputLayout(inputElementDescription, 2, shaderCode->GetBufferPointer(), shaderCode->GetBufferSize(), &vertexShader.inputLayout); shaderCode->Release(); vertexShaders[id] = vertexShader; //Place into the VertexShader map. Places copy? return vertexShader; } Now at the end of the method, right as the return is done, I see that the destructor for my variable vertexShader gets called. And this might sound ridiculous, but I really dont understand why?
      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?
      //Declaration of the vertex shader. Done at the class level VertexShader vShader; //Method that uses the createVertexShader void GraphicsSystem::initialize(HWND windowHandle) { /* Other init code*/ vShader = shaderModule.createVertexShader(L"default.shader", "VertexShader", "VShader", "vs_4_0"); }  
    • 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.
      Advice about other things are also appreciated.  Thanks.
      Edit: Hmm... I start to think that grouping component is OK, but I should still use 2 function (from different service/system).
      Edit2: fix some code (pointed out by 0r0d, thank)
    • By noodleBowl
      I'm looking into using raw input on windows for keyboard/mouse detection and I see there are const values for keys such as VK_SHIFT, VK_NUMPAD5, VK_F7 available for me to use but I have no constants for keys like A, S, D, W, 1, etc.
      Even on the windows documentation those keys have no associated constant, it only shows the key's HEX value.
      So my question is am I missing something like a header file that has these virtual key const defined or is this done on purpose? If it is done on purpose what is the reasoning behind it?
  • Popular Now