07_07: Refactoring Bad Boids change in logic and fixture that we test against

[a-doering]

While refactoring the Bad Boids, I noticed that in this commit on the branch that is demonstrating the refactoring the logic of the program is changed. Due to the change in the logic, the fixture against which we test was changed as well. I believe that this should not happen.

[jack89roberts]

After thinking about this for a while, I believe both the implementation on main and better_boids are probably technically wrong (but in slightly different ways). I believe the logic should be to:

1. Compute all the velocity deltas (without applying the updates to any of the boids).
2. Update the velocities and positions.

In the implementation on the better_boids branch that could look something like this (there's probably a nicer way to do it):

def update(self):
    delta_v = []
    for i, me in enumerate(self.boids):
        delta_v.append(array([0.0, 0.0]))
        for him in self.boids:
            delta_v[i] += me.interaction(him)
    for i, me in enumerate(self.boids):
        # Accelerate as stated
        me.velocity += delta_v[i]
        # Move according to velocities
        me.position += me.velocity

But the main point is we shouldn't have a refactoring exercise where the results of running the script change before/after.

[a-doering]

While refactoring I'm already getting an assertion error when changing the following lines:

def update_boids(boids):
    xs,ys,xvs,yvs=boids
    # Fly towards the middle
    for i in range(len(xs)):
        for j in range(len(xs)):
            xvs[i]=xvs[i]+(xs[j]-xs[i])*0.01/len(xs)
            yvs[i]=yvs[i]+(ys[j]-ys[i])*0.01/len(xs)
-   # Fly away from nearby boids
-   for i in range(len(xs)):
-       for j in range(len(xs)):
            if (xs[j]-xs[i])**2 + (ys[j]-ys[i])**2 < 100:
                xvs[i]=xvs[i]+(xs[i]-xs[j])
                yvs[i]=yvs[i]+(ys[i]-ys[j])
-   # Try to match speed with nearby boids
-   for i in range(len(xs)):
-       for j in range(len(xs)):
            if (xs[j]-xs[i])**2 + (ys[j]-ys[i])**2 < 10000:
                xvs[i]=xvs[i]+(xvs[j]-xvs[i])*0.125/len(xs)
                yvs[i]=yvs[i]+(yvs[j]-yvs[i])*0.125/len(xs)
    # Move according to velocities
    for i in range(len(xs)):
        xs[i]=xs[i]+xvs[i]
        ys[i]=ys[i]+yvs[i]

[jack89roberts]

Yeah, removing the Try to match speed with nearby boids loops will change the update results (I think the original regression test should still pass if you remove only the Fly away from nearby boids loops). The terms like (xvs[j]-xvs[i]) in the match speed with nearby boids loop are what cause the behaviour to change if you remove that one.

I guess this shows a) that via refactoring you may uncover bugs in the code that were originally missed, and b) that refactoring can have unexpected consequences!

[a-doering]

This is indeed a very interesting study topic for a learner like me! You are right about the Fly away from nearby boids loops. Perhaps you should consider granting the participants a bit more time for this exercise or encourage them to do the refactoring later, as this stayed unnoticed for a while.

[jack89roberts]

Yeah definitely, we originally planned to spend the whole of the last session on this exercise on Tuesday but ran way over time (we're also planning to add more exercises to the second week in general).

[jack89roberts]

I've reimplemented the bad_boids repo here: https://github.com/jack89roberts/bad-boids , with changes to both the main and better_boids branches so the refactoring doesn't require the tests to change. Plus added a few extras like automated testing with GitHub actions.

I'll propose we replace the current one in the Turing organisation with this, but will leave it a while in case that causes problems for anyone currently trying with the old version.