3
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ you could also consider throwing an IllegalArgumentException, if you want to make it clear that you only want to process contacts between 2 players. \$\endgroup\$ – njzk2 Jun 4 '15 at 16:24
  • \$\begingroup\$ I think this is not case to throw exception, since I have a lot of objects in my game (represented by Body) and a lot times a contact will not be between players object. But somehow you're right since the method name make us think it's an exception when Contact fixtures are not both players. I think I have to make the veriifcation out his method or change the name to processContact or something like. \$\endgroup\$ – alexpfx Jun 4 '15 at 17:04
  • 3
    \$\begingroup\$ The method name explicitely says the contact is between players, which would tend to indicate that it expects the input to be filtered before. \$\endgroup\$ – njzk2 Jun 4 '15 at 17:22
  • \$\begingroup\$ Could you tell us what save(bodyB) does, and what its return value means? \$\endgroup\$ – 200_success Jun 4 '15 at 19:54
  • \$\begingroup\$ actually this code is only for prototype purposes, probably save method will be removed. but it means something like: there is a last chance to body don't be destroyed, if 'save' returns true. Like: it returns true if a random number are greater than 0.5. \$\endgroup\$ – alexpfx Jun 5 '15 at 0:55
10
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

The first early return is fine.

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

This immediately tells the programmer that both bodies must be players.

The later ones less so, You can invert the condition and put the markForDestroy call in the then clause.

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

I would add some blank lines to break up the function a bit though:

private void processContactBetweenPlayers(final 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)) 
            markForDestroy(bodyB);
    } else {
        if (!save(bodyA))
            markForDestroy(bodyA);
    }
}
\$\endgroup\$
2
\$\begingroup\$

Here is how I would simply this:

First, I would create a reduced method for getting "body" in fixture as such:

contact.getBody("A");

Underneath the covers this could be

contact.getFixtureA().getBody()

But by doing it this way you

  1. Shorten the code
  2. Create a type of factory method for these "fixtures". This may not be appropriate for your case but with no other context that is what I would do

And, what's with all the

final

Craziness? You only need to do this if you want to assure yourself (or the compiler) that it won't be reassigned. This is hyper defensive and, in 15 years of writing code I have never found it to be a necessity.

Frankly, 'final' in this case is repetitive and distracting. Unless you have a need for it (which, from this code, you don't) then ditch it.

this:

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

is good, but this:

    if (!isPlayer(bodyA) || !isPlayer(bodyB)) {
        throw new IllegalArgumentException("Both bodies must be players!");
    }

Is way better! Think about it: In the previous case, you 'return' and terminate the function. But, in this condition you have no idea if the job was done or not and if not, why not. This is pre-condition checking and an ideal case for it

    final float vx_a = Math.abs(bodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(bodyB.getLinearVelocity().x);

Your variable names mean zilch to me. You HAVE to give them a better name. Also, create them up in groups of 2.

This code here is also problematic.

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

The return statements are pointless here. You could just do:

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

And now, a line is gone and your code flows naturally and doesn't abruptly exit the function.

Finally, this thing where you put your

return

on the same line is both cluttering and unnatural; They are sequential instructions to the CPU:

mv e248484 EAX
cmp 0
JMP ...
POPS

Ok, yes that is 80686 assembly but you get the point. It's a branch and function return.

All, of course, my .02 USD

\$\endgroup\$
  • \$\begingroup\$ I disagree with the stringly typed function. Instead if you still want only one function to get either then it should be an int. \$\endgroup\$ – ratchet freak Jun 4 '15 at 22:21
  • \$\begingroup\$ @ratchetfreak huh? \$\endgroup\$ – Christian Bongiorno Jun 4 '15 at 23:06
  • \$\begingroup\$ I'm talking about contact.getBody("A") no need for the magic string, use index of enum instead. \$\endgroup\$ – ratchet freak Jun 4 '15 at 23:17
  • \$\begingroup\$ @ratchetfreak I don't know what 'index of enum' you're referring to. Did I miss some code? This follows the factory pattern and allows the body to be polymorphic. If there is an enum to be used, then, ok. \$\endgroup\$ – Christian Bongiorno Jun 4 '15 at 23:36
  • \$\begingroup\$ typo, I meant "index or enum" \$\endgroup\$ – ratchet freak Jun 4 '15 at 23:43
2
\$\begingroup\$

processContactBetweenPlayers is not a great name. A good name should describe what the method does, so the readers can make a good guess without studying the implementation. "process" is a wishy-washy term that doesn't mean anything.

This method also knows too much about the internal workings of a Contact object. By the law of Demeter, it would be good to avoid making chained references to contact.getFixtureA().getBody(). It would be better to pass two instances of Body to this method. Then the method could be renamed to:

private void collideBodies(Body bodyA, Body bodyB) { ... }

Still about naming, it's a common practice to prefix method names that return a boolean value with is or has. Judging by your posted revised version, it would seem that isSaved would be a better name than save.


Since the calculation logic of the "power" is the same for both bodyA and bodyB and you have it repeated twice, it would be good to move this to a helper method. That would have the additional positive effect of hiding from this method the logic of calculating power, making it more cohesive, focusing on handling the collision, and not doing additional calculations.


In this condition you have some redundant parentheses:

    if ((a_power > b_power)) {
\$\endgroup\$
  • \$\begingroup\$ I aggree 'process' is not good name, but there are other types of colisions between other types of bodies that will be treated. \$\endgroup\$ – alexpfx Jun 5 '15 at 1:32
  • \$\begingroup\$ Of you had included those in your post, this would have been a different answer ;-) \$\endgroup\$ – janos Jun 5 '15 at 4:40
2
\$\begingroup\$

I find it odd that the "power" is defined as the sum of the horizontal and vertical components of the velocity. I would expect it to be based on the speed, which is \$v = \sqrt{v_x^2 + v_y^2}\$. More specifically, I'd expect to compare the kinetic energies of the bodies, \$\frac{1}{2} mv^2\$.

Therefore, I would expect to see

if ((vx_a * vx_a + vy_a + vy_a) > (vx_b * vx_b + vy_b + vy_b)) {
    …
} else {
    …
}

If you are instead modelling physics in some bizarro world where movement along the horizontal and vertical axes is considered less powerful than diagonal movement, then there should be an explanatory comment in the code.

\$\endgroup\$
  • \$\begingroup\$ It's not so important. I doing this simple calcule to see my bodies being destroyed for now, but that code will be changed. \$\endgroup\$ – alexpfx Jun 5 '15 at 1:28
-2
\$\begingroup\$

After answers and comments I refactoring my code spliting the code in several methods.

private void processContact(Contact contact) {
    final Body bodyA = contact.getFixtureA().getBody();
    final Body bodyB = contact.getFixtureB().getBody();
    if (areBothPlayers(bodyA, bodyB)) {
        processContactBetweenPlayers(contact, bodyA, bodyB);
    }
    // others contacts processing.
}

private void processContactBetweenPlayers(Contact contact, Body playerBodyA,     Body playerBodyB) {
    final float vx_a = Math.abs(playerBodyA.getLinearVelocity().x);
    final float vx_b = Math.abs(playerBodyB.getLinearVelocity().x);
    final float vy_a = Math.abs(playerBodyA.getLinearVelocity().y);
    final float vy_b = Math.abs(playerBodyB.getLinearVelocity().y);

    final float a_power = vx_a + vy_a;
    final float b_power = vx_b + vy_b;

    if (a_power > b_power) {
        if (!saved(playerBodyA)) {
            destroyIfNotSaved(playerBodyA);
        }
    } else {
        if (!saved(playerBodyB)) {
            destroyIfNotSaved(playerBodyB);
        }
    }
}

public void destroyIfNotSaved(Body playerBody) {
    if (!saved(playerBody)) {
        markForDestroy(playerBody);
    }
}

private boolean areBothPlayers(Body bodyA, Body bodyB) {
    return isPlayer(bodyA) && isPlayer(bodyB);
}
\$\endgroup\$
  • \$\begingroup\$ Using so many small methods, it's fine, but now it really cries for a method float power(playerBody). +++ You forgot to use destroyIfNotSaved. \$\endgroup\$ – maaartinus Jun 4 '15 at 22:21
  • \$\begingroup\$ ChristianBongiorno and maaartinus, I see, I will correct the code. \$\endgroup\$ – alexpfx Jun 5 '15 at 0:58

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.