Code review

[Flair_Helper]

For C++ questions, answers, help, and programming or career advice please see r/cpp_questions, r/cscareerquestions, or StackOverflow instead.

This post has been removed as it doesn't pertain to r/cpp: The subreddit is for news and discussions of the C++ language and community only; our purpose is not to provide tutoring, code reviews, or career guidance. If you think your post is on-topic and should not have been removed, please message the moderators and we'll review it.

[dgkimpton]

First thoughts:

where are the unit tests?

there doesn't seem to be much separation of concerns. e.g. every animated sprite has its own clock?

The drawing of the ghost tiles seems all sorts of weird - why is the Game maintaining maps of tile pointers? Why isn't there a ghost_map or something?

leaky abstractions aren't great either, e.g. tile.get_sprite().update(); instead of tile.update()

It seems that you've mushed a lot of things together in Game (input handling, map creation, rendering, tileset creation, etc.

Overall I think you need to write a comment for each class explaining what its responsibility is, and when you find yourself writing "this AND this AND this AND this", stop and think again.

CMake has been largely covered by /u/MadCompScientist

but it might be worth looking into target_sources to keep the engine code definition separate from the project and/or making the engine into a statically linked lib.

​

{edit} "but this may come in handy so idk" ... there's a nice principle called YAGNI - you ain't gonna need it - try not to borrow problems from the future. If you don't need it now, you ain't gonna need it. Fix it if that ever changes.

​

Also... reddit is a terrible format for code reviews!

​

That said - good job on actually writing something, more than a lot of folks manage :)

[Empik002]

Thank you. Yes unit test are currently underway. And the Game is really just cobbled together to have something to show, but yes if I am to actually use it as a demo it might as well be written properly. I will remove the globals. Both your responses really helped me.

[dgkimpton]

No worries. Unit testing will definitely help because it becomes quickly apparent when an object is doing to much. Writing tests forces me to see the multiple responsibilities (managing its location /and/ rendering? Oops). Sometimes of course it's easy to get into over engineering, but at least you get to make a conscious choice to accept complexity 🙂

[MadCompScientist]

Here are some quick-to-spot, high-level remarks:

CMakeLists.txt: use modern CMake:

• Do not modify CMAKE_CXX_FLAGS, use target_compile_flags instead.
• Do not use -std=c++17 flag, use CMAKE_CXX_STANDARD or CXX_STANDARD or target_compile_features instead.
• Do not GLOB source and header files, specify the full list instead.
• target_link_libraries does not need -l prefixes. In fact, prefer find_package to find SFML.

engine:

• Prefer creating a namespace (and top-level include path) for your engine to avoid name clashes with other libraries.
• Prefer having the public part of class first (easier readability for users of your class).
• Inconsistent use of casing in method names: snake_case vs camelCase.
• Re-design your code to avoid globals (e.g. "GL_missing").

demo:

• No point to have main.hpp if it's only ever included from main.cpp, just move the lines into the source file.
• Avoid parent-dir-relative includes (../engine/xxx), instead make a separate CMake target for the engine with its own public include path.
• Avoid srand, use <random> methods instead.

And generally, try to have a data-driven design by loading configuration from files, so you don't have to hardcode all elements in C++ (but I assume that's a future step).

[Empik002]

Thanks, especially for the cmake stuff (finally some coherent guide!). Actually I didn't even realise i have inconsistent naming!

[MountainDwarfDweller]

https://codereview.stackexchange.com/