1
\$\begingroup\$

I have a chunk of code that's working properly but it's way too complicated to read:

addCollision (bodyA, bodyB) {
    var bodyC, i3;
    if (bodyA.collision === null && bodyB.collision === null) {
        bodyA.collision = this.collisions.length;
        bodyB.collision = bodyA.collision;
        this.collisions.push([bodyA, bodyB]);
    } else if (bodyA.collision !== null && bodyB.collision === null) {
        bodyB.collision = bodyA.collision;
        this.collisions[bodyA.collision].push(bodyB);
    } else if (bodyB.collision !== null && bodyA.collision === null) {
        bodyA.collision = bodyB.collision;
        this.collisions[bodyB.collision].push(bodyA);
    } else if (bodyA.collision !== bodyB.collision) {
        var bodyBCollisionIndex = bodyB.collision;
        for (i3 = 0; i3 < this.collisions[bodyB.collision].length; ++i3){
            bodyC = this.collisions[bodyBCollisionIndex][i3];
            if (bodyC !== bodyB) {
                bodyC.collision = bodyA.collision;
                this.collisions[bodyA.collision].push(bodyC);
            }
        }
        this.collisions[bodyB.collision].length = 0;
    }
}

Basically, it's called when two bodies collide. It's main purpose is to create a new array containing the two bodies that just collided and add it to another array containing all the collisions in that frame.

Actually, it does a little more too: If one of the two bodies was already colliding with a third body before, the code will not create a new array for the collision, but instead add the new colliding body to the already existing collision array. If both bodies were already colliding with other bodies, then it should merge the collision arrays without letting any duplicates.

How can I make this code more clear?

\$\endgroup\$
1
\$\begingroup\$

If statements respond to truthy values

I am not a javascript expert (so I apologize if i've messed up the syntax somewhere), but in javascript the 'if' statement responds to truthy values:

i.e. if (examStarted) { GetOutOfBedFAST(); }

you don't need to write: if (examStarted == true ) etc

hide details behind methods so it's easy to read

addCollision (bodyA, bodyB) 
{
    var bodyC, i3;

    if (    noCollision(bodyA, bodyB)) 
    {
        coolName1(bodyA, bodyB)
    } 
    else if (ACollidesNotB(bodyA, bodyB) ) 
    {
        coolName2(bodyA, bodyB)
    } 
    // etc etc
}

noCollision(bodyA, bodyB)
{
    return (bodyA.collision === null && bodyB.collision === null);
}

coolName1(bodyA, bodyB)
{
        bodyA.collision = this.collisions.length;
        bodyB.collision = bodyA.collision;
        this.collisions.push([bodyA, bodyB]);
}

ACollidesNotB(bodyA, bodyB)
{
    return (bodyA.collision !== null && bodyB.collision === null);
}

coolName2(bodyA, bodyB)
{
       bodyB.collision = bodyA.collision;
        this.collisions[bodyA.collision].push(bodyB);
}

missing abstractions?

You don't really want to be writing code like this: if then do this, else if then do that, elseif then do another. This type of code should be avoided.

What you want to be writing, in this type of sitation is things like this:

```
Body b1 = Factory.ReturnBody();
b1.RespondToCollision();
```

Something like that. Use objects to send messages to each other. That way your code will be more OOP and easier to change when the time comes.

\$\endgroup\$
  • \$\begingroup\$ Looks good! The only problem with this approach I can think of is performance, since this function is a method from a class, and used as a callback for another function, that goes inside another method; so if I keep subdividing things into functions I'm probably going to have problems in the near future. Probably the approach I'm going for anyway. \$\endgroup\$ – Tiago Marinho May 29 '17 at 17:43
  • \$\begingroup\$ All of the above code is a response to a collision. which collision response should occur? that's something a factory should determine. it should choose the appropriate class. what is the actual collision response? that's something that should be implemented by a specific class. personally that's what i would do. it's easier to change and understand. \$\endgroup\$ – BKSpurgeon May 29 '17 at 23:03
  • \$\begingroup\$ here something like this is an option: notice the use of a factory method which chooses a particular implementation? bkspurgeon.github.io/BKSpurgeon.github.io/fizzbuzz-kata \$\endgroup\$ – BKSpurgeon May 29 '17 at 23:04
  • \$\begingroup\$ @TiagoMarinho: This comment makes absolutely no sense: I keep subdividing things into functions I'm probably going to have problems in the near future. Your functions should always be as small as possible. It does not matter how big the call stack is. Each function should have one very specific purpose. Doing lots of things in a single function is bad practice and defeats the spirit of functions. \$\endgroup\$ – I wrestled a bear once. Jul 28 '17 at 13:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.