patternjavaModerate
Conditionally eliminating one of two colliding bodies
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:
The function returns if both bodies are not players. But doing maybe this practice reduces the code readability. What do you think?
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.
This is an early return, saving you one indentation level.
This is not as good. Here, I'd go for
Note that braces should be used around single statements as well, i.e.,
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,
in case it's
While asserts are often frowned upon, this is exactly what they're good for.
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 likecheckArgument(isPlayer(bodyA) && isPlayer(bodyB));in case it's
public, otherwise I'd recommendassert 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.