U++ forum: Welcome to the forum

Re: SurfaceCtrl, 3D viewer of Multiple file format and Surface class [message #54832 is a reply to message #54824]

Sun, 20 September 2020 00:11

Klugier
Messages: 1100
Registered: September 2012
Location: Poland, Kraków

Senior Contributor

Hello Xmeuth,

Seems that the license problem is handled correctly. Thanks!

I have few remarks to the code:
- In Definition.h "#define GL GLCtrl" this is not needed explicit GLCtrl is fine and additional define can be pass to the user.
- "enum Camera_Movement {CM_FORWARD,CM_BACKWARD,CM_LEFT,CM_RIGHT};" modernized to enum class you will much better type control.
- Camera_Movement CamerMovementDirection shold be enough. I think _ is not needed in public API.
- noexcept - why it is used in so many places. I think it obscures readability and can be ommited. I do not see noexcept such common in uppsrc.
- "void MoveAllSelectedObjects(glm::vec3 move)noexcept; //Move all selected object" - this comment is redurdant to the method signature (it occures in many places). If you want documentation just use Topic++.
- Long functions are implemented in the header file. If function is not one liner then it should be moved to .cpp file.

For example: "OpenGLProgram& SetBool(Upp::String name, bool value)noexcept{if(linked)glUniform1i(GetUniformLocation(name ), (int)value);return *this;}" should be moved to .cpp file.

- Shader managment can be placed to separate package. I know we have some shader related stuff in GLDraw, but would be nice if we can unify this. Please check the code behind it
- public method/members should go first - then you should place private variables. The first thing I want to examin is the class public API. I do not want to analyze implementation
- French comments "//Gestion of alpha" - anyway in the modern programming comments should be avoided - your code should be clear enough to exist without comments. Of course, sometimes this rule may be deviated from, but in very rare cases.
- Magic numbers - what is 20000. For more info please visit https://clang.llvm.org/extra/clang-tidy/checks/readability-m agic-numbers.html. Optimally you should define constexpr variables.
- Probably more, but let's start from addressing above issues...

Klugier

U++ - one framework to rule them all.

[Updated on: Sun, 20 September 2020 00:13]

Report message to a moderator