search

jyanar / Boids

An implementation of the Boids program using C++ and the SFML library.
MIT License
33 stars 13 forks source link

Boid getBoid(int i) should return a reference not a copy. #3

Closed Aeryes closed 3 years ago

Aeryes commented 3 years ago

Boid getBoid(int i); returns a copy of the boid which makes it impossible change. If this was intended maybe adding a getter for both read only and read/write would be good.

should be:

Boid &getBoid(int i);

with the definition being:

Boid &Flock::getBoid(int i)
{
    return flock[i];
}
ufrshubham commented 3 years ago

To be honest, Flock::getBoid() should not even exist, because vector<Boid> flock is anyways public.

Aeryes commented 3 years ago

To be honest, Flock::getBoid() should not even exist, because vector<Boid> flock is anyways public.

Right but the issue is its used elsewhere in the code. So it should be made private along with the suggested code above.

ufrshubham commented 3 years ago

I understand where you are coming from, but when I see the code, all the uses of getBoid() are valid, because none of them is trying to modify the returned boid. Returning unnecessary copies is bad, but returning a non-const reference to internal data is worst IMHO.

But in any case, since you already know what you want, maybe you should create a pull-request with the change you are suggesting. Maintainer are the best judge for such changes.

ufrshubham commented 3 years ago

To be honest, Flock::getBoid() should not even exist, because vector<Boid> flock is anyways public.

Right but the issue is its used elsewhere in the code. So it should be made private along with the suggested code above.

Btw, I am working on re-creating this project with better architecture here: https://github.com/ufrshubham/Boids-SFML

Aeryes commented 3 years ago

To be honest, Flock::getBoid() should not even exist, because vector<Boid> flock is anyways public.

Right but the issue is its used elsewhere in the code. So it should be made private along with the suggested code above.

Btw, I am working on re-creating this project with better architecture here: https://github.com/ufrshubham/Boids-SFML

Awesome. I think it would be cool to add some food to the simulation also. I'm using this project as part of a larger evolution style project that I am working on where flocking is one of my simulations evolutionary traits.. The biggest issue I have with this project is the movement. I think a better way to structure the Boid movement would be to have a flock leader and align the direction of all boids to that leader and allow the leader a larger scope for movement. As it stands right now, the boids move in the same direction and rarely change to go in the opposite direction.

chernandez7 commented 3 years ago

Hey, thanks for the comments. This project was originally made in college while first learning C++ so there's a lot of noob mistakes. I personally have no issues with refactoring code or reviewing pull requests as long as it improves the codebase :)

Aeryes commented 3 years ago

Hey, thanks for the comments. This project was originally made in college while first learning C++ so there's a lot of noob mistakes. I personally have no issues with refactoring code or reviewing pull requests as long as it improves the codebase :)

Awesome. Sounds good. It's a nice project. I'm using it right now for an evolution based simulation I'm working on. I'll send you the link to the project when it's done. I've added credits to each of my headers file linking to the code of yours that I've used.

jyanar commented 3 years ago

Yep, thanks for adding these small fix-ups. Sorry about the delay, the end of the semester is always a crazy time. Will merge your pull request. #4