HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Conditionally eliminating one of two colliding bodies

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conditionallybodieseliminatingonetwocolliding

Problem

I like to explicit return function even for void functions to reduce the use of if-elses, like this code snippet:

private void processContactBetweenPlayers(Contact contact) {
    final Body bodyA = contact.getFixtureA().getBody();
    final Body bodyB = contact.getFixtureB().getBody();
    if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
        return;
    }
    final float vx_a = Math.abs(bodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(bodyB.getLinearVelocity().x);
    final float vy_a = Math.abs(bodyA.getLinearVelocity().y);
    final float vy_b = Math.abs(bodyB.getLinearVelocity().y);
    final float a_power = vx_a + vy_a;
    final float b_power = vx_b + vy_b;
    if ((a_power > b_power)) {
        if (save(bodyB)) return;
        markForDestroy(bodyB);
    } else {
        if (save(bodyA)) return;
        markForDestroy(bodyA);
    }
}


The function returns if both bodies are not players. But doing maybe this practice reduces the code readability. What do you think?

Solution

IMHO this is perfectly fine.

if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
    return;
}


This is an early return, saving you one indentation level.

if (save(bodyB)) return;
    markForDestroy(bodyB);


This is not as good. Here, I'd go for

if (!save(bodyB)) markForDestroy(bodyB);


Note that braces should be used around single statements as well, i.e.,

if (!save(bodyB)) {
        markForDestroy(bodyB);
    }



But doing maybe this practice reduces the code readability.

No, used wisely it helps readability. When reading top down, you know that both bodies are players. In an if-block you know this too, but the indentation may get excessive. Moreover, using the early return makes clear that only players are dealt with.

As stated in the comments, processContact would be a much better name. A method called processContactBetweenPlayers could be extracted (not needed now as it's rather short). Such a method should use something like

checkArgument(isPlayer(bodyA) && isPlayer(bodyB));


in case it's public, otherwise I'd recommend

assert isPlayer(bodyA) && isPlayer(bodyB);


While asserts are often frowned upon, this is exactly what they're good for.

Code Snippets

if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
    return;
}
if (save(bodyB)) return;
    markForDestroy(bodyB);
if (!save(bodyB)) markForDestroy(bodyB);
if (!save(bodyB)) {
        markForDestroy(bodyB);
    }
checkArgument(isPlayer(bodyA) && isPlayer(bodyB));

Context

StackExchange Code Review Q#92660, answer score: 10

Revisions (0)

No revisions yet.