Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seeking Code Review Feedback #1

Open
nhurde opened this issue May 20, 2024 · 1 comment
Open

Seeking Code Review Feedback #1

nhurde opened this issue May 20, 2024 · 1 comment
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@nhurde
Copy link
Contributor

nhurde commented May 20, 2024

Please comment below for any changes for next version you may see within the single header file.

@FtZPetruska
Copy link

After giving it a quick look, here's a list of things in no particular order.

  • Add code examples. I would personally expect the examples directory to contain code that shows how you got the resulting gifs, even just to illustrate how you'd use the functions.

  • Add some protection against symbol redefinition. Currently, the header lacks any form of include guard, and defines no way to hide implementation. About that latter point, it's typical for header-only libraries in C to have the implementation of functions hidden behind a MYLIB_IMPLEMENTATION define with the documentation stating something along the lines of:

Do this:
      #define MYLIB_IMPLEMENTATION
   before you include this file in *one* C or C++ file to create the implementation.
  • Is this supposed to work with C ? It is a .h file, but uses things like the cmath header, the bool type, and default function arguments. If C support is desired, SDL offers its own math functions and boolean type.

  • The naming is... Odd ? Some functions are prefixed with SDL2_RenderParty_, while others lack the 2. I am also, personally, not fond of starting function names with SDL_ because it feels like you're stepping in SDL's "namespace". So maybe have the functions be prefixed with SRP_ or just RP_, but that's very much a matter of taste.

  • Support for some buildsystem. While this is a header-only library and does not need to be built, I do think something like a simple CMake script could be useful:

    • If code examples/tests are added, then it provides the scaffolding to build and run them.
    • Make installing the header (and other files if needed) trivial.
    • You can express the requirements to consume the header, such as the SDL2 dependency.

@nhurde nhurde added the enhancement New feature or request label May 22, 2024
@nhurde nhurde self-assigned this May 22, 2024
@nhurde nhurde moved this to 👀 In review in Open Source RoadMap May 22, 2024
@nhurde nhurde added this to the Summer 2024 milestone May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 👀 In review
Development

No branches or pull requests

2 participants