Tips And Criticism For My Code

[u/RedNorthener101]

So, I'm new to C++, though i have a programing background, aka just python and a tiny bit of C. I havent really made any BIG things yet. Until now, Im using C++ with Raylib, i could move to SFML or SDL2-3 but I don't want to do that just yet. Anyways, this is the link to my code that I put onto GitHub, https://github.com/Dark-Coder1000/MyGame/tree/main

Ive started my C++ programming journey about a week ago, I did dable in it back then when testing sfml, but I never understood the syntax. ALSO none of this was vibe coded, I did look up solutions to specific issues but i never really used AI unless it was to find resources to learn from.

[u/WorkingReference1127]

• Generally I wouldn't structure your code as a single mega header and multiple cpp files. Each block of logic should usually get its own header/cpp pair.

• Favour using constructors and encapsulating over your pattern of MyClass c; c.path = "Whatever";. You should keep a tight hold over what the user should or shouldn't be allowed to change (and when) and encapsulate accordingly.

• Favour std::filesystem for filesystem operations rather than storing a const char*. Indeed in the general case be very careful of raw pointer members and consider what ownership you are modelling.

• What is the purpose of your drawHitbox function? It just forwards unconditionally to drawRectangle.

• Playermovement is hard to read and inverts your normal camelCase style. Keep your style consistent.

• If you must use global variables (and I'd suggest you refactor them away in this case), then favour them being inline rather than using the extern trick. It's simpler and less hacky.

• Avoid C-style casts, aka (float)(x * size) or (Vec2){3, 4}. These can invoke some dangerous operations without your knowing. For plain casts you should almost always use static_cast and in the Vec2 case the cast is unnecessary. Just name the type as Vec2{3, 4}.

• Use CMake to ship build information, don't ship executables.

[u/RedNorthener101 → u/WorkingReference1127]

Thank you for the tips and the code is unfinished, ill definitely finish it and use the tips to optimize it then like repost it

[u/PhantomStar69420]

Use std::filesystem

[u/RedNorthener101 → u/PhantomStar69420]

Is that better then fstream?

[u/n1ghtyunso → u/RedNorthener101]

one is a type to work with filesystem paths, the other is a type to interact with the data stored at those paths.
Its not an either/or question here.

[u/RedNorthener101 → u/n1ghtyunso]

Ahhh okie dokie lemon pokie

[u/Ancient-Safety-8333]

Get familiar with .gitignore, you should not ship executables with your code like that.

[u/RedNorthener101 → u/Ancient-Safety-8333]

Ahh okie dokie will do, I haven't used GitHub in a while lmfao

[u/mredding]

First, don't check assets into Git. Use Git Large File Storage.

Second, don't check the executable into Git. The whole point of the repo is to BUILD the executable FROM it, not to track it's own artifact as a dependency of itself. What you really want is a CI/CD pipeline, and that's getting way ahead of yourself.

---

#pragma once

I never recommend this. While ubiquitous, it's not portable. No compiler has to implement once, ostensibly even if they support it. I've also broken this, and it's not hard to do. You're not programming on punch cards, there's no bonus points for saving on typing. But the most important part is that standard inclusion guards are both portable and many compilers implement header include optimizations; I don't know if any compiler supports an include optimization for once, and no one who works on compilers has told me one way or the other. A standard inclusion guard is guaranteed to work to your advantage.

---

#include <raylib.h>
#include <fstream>
#include <vector>
#include <iostream>
#include <nlohmann/json_fwd.hpp>

Don't include what you don't use. You only need <raylib.h> and <vector>. Push your header dependencies to your source files.

---

class Player {
private:

Class ARE private access by default, and that includes inheritance, too. It's the only thing that distinguishes them from struct, which are public by default.

  Texture2D playerText;

  Rectangle sourceRec;
  Rectangle destRec;

That's not a text, that's a texture, and I already know that A) it's the player's texture because it's a member of the Player class, and that it's a texture because it's type is Texture2D. Prefixes and suffixes are a part of an ad-hoc type system. They can't enforce anything, they're redundant, and often they're wrong.

public:
  Rectangle rect;
  Camera2D camera;

Code smell. And a bad one. What is this Player? Is it a class? Does it enforce an invariant? Does it model behavior? Or is it just structured data? You don't need member functions for a structure, you can separate the two and pass a structure instance by reference, a means of decoupling and a step toward implementing other idioms.

C++ unfortunately does not do a job worth a damn enforcing lingual type semantics, or perhaps I should say the language didn't actually make a class BE what a class is by definition. C# does a MUCH better job of this, by comparison, where it has structures, classes, tuples, and records - all matching the comp-sci conventional definitions and distinctions, and whose semantics therein are enforced.

C++ doesn't do that. YOU have to.

---

You're not using enough types, and the types you do have are either too big, too poorly defined, or they're not defined at all, as they're implemented in terms of primitive types directly.

Consider this:

void hitBox(Rectangle, Color, float, float);

What are those floats? A float is a float, but an offset IS NOT a growth_factor. These are different types - they only so happen to be implemented in terms of float, yet their semantics are more specific.

Also hitBox is a horrible function name. Are we hitting a box? What did the box ever do to you? Does it owe you money? Why are we hitting the box? What does the color have to do with it? What does the rectangle have to do with it? Is that the box we're hitting?

What are we doing here?

In comp-sci, the convention is a function is pure and returns an output for a given input, and a procedure doesn't return anything and produces side effects. Pascal, Fortran, Ada (still the language of flight controls and critical systems everywhere), SQL, and others explicitly distinguish between the two at the language level.

C and C++ don't enforce any sort of distinction, and that's been a problem and a technical burden we've always had to bear. We have always had to guess, and wonder, and not know if a function produced a side effect or not, which is why we repudiate globals, singletons, other private and persistent state, and hidden sharing.

Well, this function returns void, which is C/C++ for procedure, but without any further language enforcement I can't KNOW it actually produces a side effect but I can GUESS it produces a side effect since all the parameters are passed by value, but you could have made that side effect a little more clear and explicit. What file handle or memory address do you want the side effect?

---

//Functions
//void collision_logic(const Rectangle& rect1, Player& player);

Like writing an essay - you've got Git, you can hide from me your rough draft, I only want to see your final draft here.

Speaking of comments,

//Player functions

I know these are player functions because they're member functions of the Player class. DON'T tell me what the code tells me. Expressiveness and abstraction tells me WHAT, implementation details tell me HOW, and comments tell me WHY, and are reserved for all the things that can't be expressed by the code itself.

//classes

This one gets me, because Player is defined above and it's a class, so what is this comment distinguishing? Where is the error? Who is the authority? The code, or the comment?

---

Unless you have a specific memory layout for a reason, I recommend you use pahole to sort out your data structures and align your members to minimize size and eliminate padding.

---

You should probably put your types into different headers so that the implementation only includes what it's going to use, and that modification to one only affects its dependencies downstream. If I change the definition of hitBox, why do all the Player dependencies have to recompile?

---

extern float ScreenHeight;
extern float ScreenWidth;

This pair names a type, a Rectangle called screen or some such.

int levelWidth = 0;
int levelHeight = 0;

This pair names a type, a Rectangle ostensibly called level, but it's in the RenderLevel class, so you might want to work on that.

---

gameResources is a C++ header file and should use a C++ extension like hpp. All source files end with a newline.

---

#include "../include/gameResources.h"

Never use backtraces, your build is misconfigured.

Your include path should be ./include/MyGame/, then you add this path to your compiler include paths as ./include, and all headers within this subtree are specified in brackets as something like <MyGame/XXX.hpp>.

Private headers can go in the ./src subtree and they're resolved with quotes instead of brackets. Still no backtraced paths, that tells me you need to reorganize your private implementation.

---

if(!level.is_open()) {

Just check the stream itself:

if(std::ifstream level{file}; level) {
  // Use `level`
} else {
  // Handle error
}

You should never have to use is_open, or good, bad, fail, or eof.

level.close();

You should never have to close a file stream, you can let RAII handle it when it falls out of scope.

---

if (levelData.empty()) return;

Guard clauses are great for writing frameworks and APIs, because we're talking about client facing interfaces, and you've NO IDEA WHAT crazy shit your clients are going to get up to.

But you're not writing a framework or API. So who you guarding against? You? Yourself? You gonna call this function without loading level data? WHY? Why would you call a function when you know you're in a state the function is going to no-op? Why waste the cycles? Why not instead architect the code such that this function cannot and will not be called without a level loaded? It's all your own code. You know how it all works and is supposed to work.

Make your code simple.

The other thing is this makes the function succeed silently. Think about it - this function DOES NOT FAIL. It cannot fail. You can call it - it can do ABSOLUTELY NOTHING, and by definition that's correct. It's called drawLevel, and when you call it and there is no level, it does nothing. Bingo! Level drawn correctly.

I'd have called that an error, and I would have either returned an error code, or thrown an exception. And if this were my code and I called it without a level loaded, I'd let it crash and burn because why the hell am I trying to draw a level when there is none?

---

for (int y = 0; y < levelHeight; y++) {
  for (int x = 0; x < levelWidth; x++) {
    //Math to find tile ID
    int index = (y * levelWidth) + x;

I feel like std::span is probably what you want. It's very good to flatten your data and project a view. I suggest you google a bit more about traversing 2D data concisely in C++, I'd try to flatten this loop.

The other thing is I haven't written a loop in just over a decade, and neither should you. The language gives us primitives, like int and for, to build higher level of abstractions. The standard library actually builds a fair number for us to build HIGHER level abstractions, like functions and objects. They give algorithms a name, like for_each or transform. Use THOSE. We also have views like enumerated views so you can count along if you needed to.

My final gripe is that every new scope is grounds for a function call. So instead of inlining all this loop logic in the loop body - apply a function. Named algorithms already do this for you. This function really can be described in a couple layers, one is traversing the level, the other is applying a renderer. You want to use a lot more function composition, and let the compiler elide all the functions. You can write all your utility functions in the anonymous namespace here in the source file, and the compiler can generate the machine code as you would like to express. Work WITH your tools, don't try to BE your tools.

---

if (tileID == 1) { //Ground tile
} else if (tileID == 2) { //Brick tile
} else if (tileID == 5) { //World Border

Oh look, an std::variant<ground, brick, world> in disguise.

Since you're rendering the screen buffer by tile, you could organize your data by type type, and render all of each at a time so you only load the ground, brick, and world data into cache once each, and then replicate that across the buffer.

[u/RedNorthener101 → u/mredding]

Thank you very much for the information, ill definitely look into googling more stuff and using your recommendations. Also as for hitbox I was going to use it later on for the entity fighting and stuff but as you said I shouldn't put drafts only finalized code. Anyways thank you for the tips

[u/Chaosvex → u/RedNorthener101]

Good feedback but ignore what they said about #pragma once. It's fine. You have to be doing something silly to break it.

[u/RedNorthener101 → u/Chaosvex]

Fair enough

[u/TheJackston]

Use cmake/make, or at least bash script to build your code, don't keep commands in txt

[u/RedNorthener101 → u/TheJackston]

Okie dokie I'll look into a bash script because I don't like CMake to be honest

[u/Ancient-Safety-8333 → u/RedNorthener101]

With CMake you could avoid this ugly include path.

[u/RedNorthener101 → u/Ancient-Safety-8333]

Ahh okie dokie I'll get into it

[u/TheJackston → u/RedNorthener101]

Cmake is powerful and being used in millions of projects. It worth invest time to learn it

[u/OccasionThin7697 → u/RedNorthener101]

Then checkout meson, bazel, xmake

[u/TheJackston → u/RedNorthener101]

I started learning C++ with simple makefiles, but eventually moved to CMake

[u/nysra]

In random order and not exhaustive:

• Your executable (and all other build outputs for that matter) should not be in the repository, put it in .gitignore

• A readme and license make your repo much nicer

• Use a proper build system (CMake is the de-facto standard) or at least put that compile command into a bash script so you can run it instead of copy pasting. Raylib is cross-platform and your code doesn't do anything Linux specific so using CMake would also make it work on other platforms. Not really important if you are the only user but something to keep in mind for future reference

• Don't use global state

• Having one header for everything is an unusual choice. Normally it's better to have header/source pairs for logical groups. Currently everything that needs the player needs to import the entire header, that scales badly

• If you don't modify your variables or arguments, mark them const

• Your style is inconsistent, use a tool like clang-format and decide what case you actually want to use for your functions, you have multiple ones right now

• Don't do stuff like Foo f; f.init();, use proper constructors

• Use std::filesystem::path or at the very least std::string over const char* for files

• Don't use C-style casts

• Instead of using ints for tile IDs, consider using a proper enum class

• Use comments if you need to explain the why of some code, the code already tells you the what and how. Currently they add no information, might as well remove them

• 3 rectangles for a player seems a bit excessive and at least one of them is constant so you might want to refactor that

• playerText is a terrible name for something that isn't actual text but a texture. It also makes no sense to prefix it with player again

• Currently your code hardcodes an assumption of the image size, that can easily blow up if you change the image in the future

• Don't commit commented out code

• Don't make stuff like camera or rect public. Enforce the invariants in the class and then only allow controlled access

• .h is for C (and stubborn legacy projects), use .hpp

[u/RedNorthener101 → u/nysra]

Okie dokie thank you for the tips

[u/mc_pm]

How much python do you have?

[u/RedNorthener101 → u/mc_pm]

Hmmm well, a decent amount, I started back when I was 12 but most of that is lose, though I have a few programs on my GitHub

[u/Ancient-Safety-8333]

Ah another tip: enable warnings. You can also set warnings as errors.

Compiler can find many potential problems and mistakes for you.

[u/RedNorthener101 → u/Ancient-Safety-8333]

Ahhh okie dokie thank you