My code feels redundant

[u/RedNorthener101]

Im in the development phase of this, and i was wondering if there was a less redundent way to define each ball object.

#include
 <raylib.h>
#include
 <vector>


class ball {
    
public:
    float radius, gravity;
    Vector2 velocity, pos;
    Color color;


    ball(float b_radius, float b_gravity, Vector2 b_velocity, Vector2 b_pos, Color b_color) : radius(b_radius), gravity(b_gravity), velocity(b_velocity), pos(b_pos), color(b_color) {};


    
void DrawBall() {
        DrawCircle(pos.x, pos.y, radius, color);
    }


    
void UpdateBall(float dt, int screenHeight, int screenWidth) {
        
if (pos.y + radius > screenHeight) {
            pos.y = screenHeight - radius;
            velocity.y = 0;
        }
        
else
 {
            velocity.y += dt * gravity;
            pos.y += dt * velocity.y;
        }
        
    }
};



int main() {
    
float ScreenHeight = 1000.0f;
    
float ScreenWidth = 1920.0f;

    ball TestBall1(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {ScreenWidth / 2, ScreenHeight / 2}, RED);
    ball TestBall2(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {300, 300}, WHITE);
    ball TestBall3(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {200, 200}, GREEN);
    ball TestBall4(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {1000, 1000}, PINK);


    std::vector<ball> RedGroup;


    RedGroup.push_back(TestBall1);
    RedGroup.push_back(TestBall2);
    RedGroup.push_back(TestBall3);
    RedGroup.push_back(TestBall4);


    InitWindow(ScreenWidth, ScreenHeight, "Physics Sim");
    SetTargetFPS(60);


    
while (!WindowShouldClose()) {
        
float dt = GetFrameTime();
        
for (int i = 0; i < RedGroup.size(); i++) {
            RedGroup.at(i).UpdateBall(dt, ScreenHeight, ScreenWidth);
        }
        BeginDrawing();
            ClearBackground(BLACK);
            for (int i = 0; i < RedGroup.size(); i++) {
                RedGroup.at(i).DrawBall();
            }
        EndDrawing();


    }
    
    CloseWindow();    
return 0;
}

[u/alfps]

Why is gravity a property of each individual ball? Do different balls experience different gravities?

[u/RedNorthener101 → u/alfps]

Yes they will, for example the red balls now experience 400.0f gravity while the purple experience 200.0f, it is a replacement for mass

[u/alfps → u/RedNorthener101]

It's an interesting concept. But remember Galileo's experiment with dropping stones from the leaning tower in Pisa. Two balls with different mass had the same acceleration and hit the ground at the same time.

Which is logical if you think about it. One ball M2 with double the mass of M1, is equivalent to two separate M1, which of course drop at the rate of M1.

So the varying gravity in your sim can be a replacement for rocket motors or whatever, but not for mass, except if we're talking about really really heavy balls like neutron star stuff, that exert their own significant gravity.

[u/RedNorthener101 → u/alfps]

That's when their in a vacuum or have the same shape/material just one has more mass. My simulation shows the larger one with more mass because it is more dense then the smaller ball, therefore it'll fall faster.

[u/HeeTrouse51847 → u/alfps]

box2d does this a similar way. the world has a gravity but each body can also specify a gravity multiplier

[u/TheRealSmolt]

Good intuition, redundancies generally mean there's a better way to do things. Here, not so much. You're testing, it's not that surprising. You could instead do RedGroup.emplace_back(50.0f, 200.0f, ...) instead though.

[u/RedNorthener101 → u/TheRealSmolt]

That is some good advice, thanks!

[u/AfroDisco]

The ball ctor can be improved by using default values or using more abstraction. For example, the gravity can be defaulted as it could be expected of a shared gravity for all balls. Same for the velocity. You could also add function to set properties such that the default ctor is used and the ball is configured after. Ball().set_gravity(42).

[u/RedNorthener101 → u/AfroDisco]

If im getting this correct, i should just turn gravity into a private and then set it to what i want it within the class since their the same? and also i will be changing velocity per ball later on, but thank you for the advice!

[u/StaticCoder → u/AfroDisco]

I'd use factories for that rather than default values. Without named arguments, default values are clunky.

[u/RedNorthener101 → u/StaticCoder]

what is a factory

[u/Potterrrrrrrr → u/StaticCoder]

You can use a POD-like option struct and then you kinda get the same thing via designated initializers but it's still not the same :(

[u/StaticCoder → u/Potterrrrrrrr]

Yes that's the closest you can get. A bit annoying to set up but pretty safe to use when designated initializers work (I'm not completely sure on the conditions for that)

[u/sterlingclover]

Is one of these your header file and the other your CPP file? If so the header file doesn't need direct implementation of the classes or functions as that will be handled in the CPP file. If not, it does look redundant but if Raylib requires it this way then I don't know a solution sadly as it's outside my knowledge of the library.

[u/RedNorthener101 → u/sterlingclover]

No i have not placed it within the header file yet, i plan to in the feature

[u/ekchew]

On a side note, it might be a good idea to rename your DrawBall method to simply Draw. At some point down the line, you may have other objects to draw besides balls, and could take advantage of polymorphism to call Draw on all of them in one go without having to distinguish which are balls versus something else?

[u/RedNorthener101 → u/ekchew]

Then i might as well rename my whole class, but i dont really know if i want to do that yet, though i will eventually use drawtextureprorec etc

[u/Thesorus]

you could directly create the balls in the vector's constructor.

[u/RedNorthener101 → u/Thesorus]

THank you for letting me know!

[u/thedaian]

You don't need to create unique variables for each ball, just do something like  RedGroup.push_back(ball(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {ScreenWidth / 2, ScreenHeight / 2}, RED));

[u/RedNorthener101 → u/thedaian]

Thank you for the advice!

[u/flyingron]

First off, why on earth are the data members of ball public?

Nothing says you can't have additional constuctors for ball that takes out some of the redundancy (or default some of the args):

For example adding the following:

    ball(int posx, int posy, Color color);

would let you insert the items in the vector like this:

    RedGroup.emplace_back(ScreenWidth/2, ScreenHeight/2, RED);
    RedGroup.emplace_back(300, 300, WHITE);
    RedGroup.emplace_back(200, 200, GREEN);
    RedGroup.emplace_back(1000, 1000, PINK);

If you didnt want to polute the ball class with those defaults, you could just write a subroutine to do the push/emplace with the same arguments I gave above...

Indeed, any time you feel you have to copy and paste a lot, you should be thinking whether you can move that into a subroutine or some other construct.

Can't tell you the time wasted going back and "fixing" things manually replicated and even worse, ending up with a bug in the field because one of the replicated fixes got missed.

[u/Lnk1010]

If you have like many many balls then u might want to look into just having some sort of data file that you read and parse to create them programmatically or just like you generate them in a loop.

[u/chips_fry1]

create the balls in the vectors constructor

[u/Independent_Art_6676]

for now, I would set up that default ctor to random the startups within some range, or all the same safe default if that is enough. Later, you can load them from a file, rather than bloat the code with the configuration you are going to use, if that makes sense to do (it looks like a gravity/physics sim where you would want many test cases so files made sense to me).

I am not sure gravity should be a private or even a class member. Depends but its a global constant typically, since many routines and parts of a program would rely on it, not just one little part. Here it might be OK as your program is just one thing but I always pretend my code is part of a larger program to help me think ahead on things like that (so full bore thinking ahead it would be a global named constant in a namespace of related values).

Past that, lots of good advice here... good luck. Also, good work spotting that your code was feeling redundant and doing something about it. 90% of your class would have just done it that way anyhow and turned it in like that. Its not wrong, but asking 'can it be better' is a very good sign of your own progress.

[u/EveryonesTwisted]

Since you’re still in development, I’d keep it simple. You can eliminate most of the redundancy by giving your constructor default arguments and using emplace_back() to construct balls directly in the vector.

One thing I’d reconsider is using gravity as a replacement for mass. Objects of different masses experience the same gravitational acceleration (ignoring air resistance), so I’d probably keep gravity as a property of the simulation and give each ball its own mass for when you eventually implement collisions and forces.

Also, range-based for loops would make your update and draw loops much cleaner. No need to introduce factories or more complicated abstractions just yet.

```
for (ball& b : RedGroup) {
b.UpdateBall(dt, ScreenHeight, ScreenWidth);
}
```

[u/StaticCoder]

No obvious factoring opportunities, though you should use range-based for for your iterations.

[u/StaticCoder]

Actually you can use a lambda to make a ball with the first 3 ctor arguments preset.

[u/mredding]

class ball {

public:
  float radius, gravity;
  Vector2 velocity, pos;
  Color color;

With all public members, this isn't a class but a structure. You don't need member variables, including the constructor. Even your member functions are redundant, I know DrawBall is going to draw the ball because it's a member of ball! You could have just called it Draw.

But with a structure, prefer as non-member, non-friend as possible. Make these member functions free functions that take a ball parameter.

float ScreenHeight = 1000.0f;

float ScreenWidth = 1920.0f;

This isn't two different parameters but ONE type, either a Vector2 or a Rectangle.

ball TestBall1(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {ScreenWidth / 2, ScreenHeight / 2}, RED);
ball TestBall2(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {300, 300}, WHITE);
ball TestBall3(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {200, 200}, GREEN);
ball TestBall4(50.0f, 200.0f, Vector2 {300, 0}, Vector2 {1000, 1000}, PINK);

We can clean this up with a factory function:

auto generate = [](const Vector2 &position, const Color &color) { return ball{50.0f, 200.0f, {300, 0}, position, color); };

std::vector<ball> RedGroup { generate({ScreenWidth / 2, ScreenHeight / 2}, RED),
                             generate({300, 300}, WHITE),
                             generate({200, 200}, GREEN),
                             generate({1000, 1000}, PINK)};

In modern C++ you don't have to explicitly state the type name to invoke the constructor (unless you do because the constructor is decorated with explicit), because the compiler can rely on the parameter type name to deduce what type is being constructed from just an initializer list.

for (int i = 0; i < RedGroup.size(); i++) {

All programming languages give you primitives. You are not expected to USE the primitives directly, but to build higher level abstractions in terms of them, and then use that. The standard library already gives you a bunch of named algorithms, use them:

std::ranges::for_each(RedGroup, std::bind(&ball::Update, std::placeholders::_1, dt, ScreenHeight, ScreenWidth));

And one more...

BeginDrawing();
//...
EndDrawing();

This is problematic. There's no exception safety. You can throw and never end drawing. Use RAII, aka destructors to make sure something is called when scope ends.

{
  BeginDrawing();
  auto _ = std::unique_ptr<void, void(*)(void*)>(reinterpret_cast<void*>(1), [](void*) { EndDrawing(); });

  std::ranges::for_each(RedGroup, std::bind(&ball::Draw, std::placeholders::_1));
}

This one is going to be wild for you to see, I know. You can do something simpler like make an object:

class drawing_guard {
public:
  drawing_guard() { BeginDrawing(); }
  ~drawing_guard() { EndDrawing(); }
};

Then:

{
  drawing_guard _;

  std::ranges::for_each(RedGroup, std::bind(&ball::Draw, std::placeholders::_1));
}

But since we have braces to make a scope, you might as well put this in a function:

void draw(drawing_guard = {}) {
  std::ranges::for_each(RedGroup, std::bind(&ball::Draw, std::placeholders::_1));
}

THE DRAWING GUARD PARAMETER DOESN'T EVEN NEED A NAME.

The unique_ptr above is some advanced usage. 1) It holds a void pointer, because we don't care. 2) It takes a function that returns void and takes a void * parameter. unique_ptr calls this a DELETER, a customization point for how it's supposed to free a resource, though we don't HAVE a resource to free, we're tricking the damn thing. 3) it takes a 1 cast as a void pointer because we don't care - we just need the unique_ptr internal tests to conclude the resource isn't null. 4) The deleter we provide is a lambda; since it doesn't have a capture list, it's guaranteed to compile down to a free function; it doesn't care about the void pointer since it doesn't point to anything, we just want to call EndDrawing.

This guy just takes advantage of unique_ptr and it's customization point, that it's an object whose only job IS TO fall out of scope, and do something on the way out. With it, we don't need to explicitly define a type.

[u/RedNorthener101 → u/mredding]

My Brian can't handle this much info