CODEWITHSUNDEEP
javaif-statement
nullUser
nullUser

Reputation: 1833

If statement hack, is there is better way to write this?

I am taking an experimental first shot at writing a simple side-scroller game. I have this code for re-centering the screen if the player moves too far in some direction.

Is there a better way to write it? I feel like I am abusing the language, but it worked out so nicely that I think it might be okay.

public void adjustFrameIfNecessary()
{
    int dx, dy;
    if ((dx = (GAME_WIDTH - GAME_WIDTH / 3) - player.x) < 0 || (dx = GAME_WIDTH / 3 - player.x) > 0 || (dx = 0) == 0);
    if ((dy = (GAME_HEIGHT - GAME_HEIGHT / 3) - player.y) < 0 || (dy = GAME_HEIGHT / 3 - player.y) > 0 || (dy = 0) == 0);

    if(dx != 0 || dy != 0)
    {
        for (Drawable shiftMe : drawables)
        {
            shiftMe.unconditionalShift(dx, dy);
        }
    }

}

EDIT

With regards to everyone's input, in an attempt to make it more readable, I have changed it to this

public void adjustFrameIfNecessary()
{
    int dx, dy;

    assignX:
    {
        dx = (GAME_WIDTH - GAME_WIDTH / 3) - player.x;
        if(dx < 0) break assignX;
        dx = GAME_WIDTH / 3 - player.x;
        if(dx > 0) break assignX;
        dx = 0;
    }

    assignY:
    {
        dy = (GAME_HEIGHT - GAME_HEIGHT / 3) - player.y;
        if(dy < 0) break assignY;
        dy = GAME_HEIGHT / 3 - player.y;
        if(dy > 0) break assignY;
        dy = 0;
    }

    if (dx != 0 || dy != 0)
    {
        for (Drawable shiftMe : drawables)
        {
            shiftMe.unconditionalShift(dx, dy);
        }
    }

}

Is this better?

EDIT 2

public void adjustFrameIfNecessary()
{
    int dx = calculateShift(GAME_WIDTH, frameReference.x);
    int dy = calculateShift(GAME_HEIGHT, frameReference.y);

    if (dx != 0 || dy != 0)
    {
        for (Drawable shiftMe : drawables)
        {
            shiftMe.unconditionalShift(dx, dy);
        }
    }
}

I think that it is clear now. Thanks everyone.

Upvotes: 2

Views: 982

Answers (5)

kritzikratzi
kritzikratzi

Reputation: 20221

the idea to improve such code is usually to use math :)

first of all, sorry for the long post. if you ever meet me in person don't ask me about this topic, i won't stop talking.

short version

do this: 

/**
 * Computes the distance from a point x to an interval [a,b]
 */
public static int distanceToInterval( int x, int a, int b ){
   return x<a? (a-x): ( x>b? (b-x):0 ); 
   // alternative: 
   // return Math.max( a - x, 0 ) + Math.min( b - x, 0 ); 
}

// then use as 
int dx = distanceToInterval( player.x, GAME_WIDTH/3, GAME_WIDTH*2/3 ); 
int dy = distanceToInterval( player.y, GAME_HEIGHT/3, GAME_HEIGHT*2/3 );

long version

your initial statement

if ((dx = (GAME_WIDTH - GAME_WIDTH / 3) - player.x) < 0 || 
    (dx = GAME_WIDTH / 3 - player.x) > 0 || 
    (dx = 0) == 0);

this can be cleaned up a bit by making it into a ...

...longer version

dx1 = GAME_WIDTH*2/3 - player.x; 
dx2 = GAME_WIDTH*1/3 - player.x; 
dx3 = 0; 

if( dx1 < 0 ) dx = dx1; 
else if( dx2 > 0 ) dx = dx2; 
else dx = dx3;

this is imho a lot clearer. i can now see what you're trying to do and put it in a neat sentence: if the player is not inside the center third of the screen, we'd like to move the screen

make it even more clear

if( player.x < GAME_WIDTH/3 ) dx = GAME_WIDTH/3 - player.x;  // positive num
else if( player.x > GAME_WIDTH*2/3 ) dx = GAME_WIDTH*2/3 - player.x; // negative num
else dx = 0; // no shift

you can see i reordered your statement, i check the left boundary first, then the right one. if you come a country that reads from right to left you might find the other direction more intuitive :)

THIS is the solution i would pick. it's short, it's readable, it's perfect :)

however, if you want it more concise you could go a step further

do the math tricks

you need to understand that the three cases are completely exclusive, they can't ever happen at the same time. let's go back to using more variables: 

// on the left side we shift a positive number, or not at all 
dx1 = Math.max( GAME_WIDTH/3 - player.x, 0 ); 
// on the right side we shift a negative amount, or not at all 
dx2 = Math.min( GAME_WIDTH*2/3 - player.x, 0 );

now look at the beauty of this:

  • if GAME_WIDTH/3-player.x > 0, then GAME_WIDTH*2/3 - player.x > 0 thus Math.min(...,0) = 0
  • if GAME_WIDTH*2/3 - player.x < 0, then GAME_WIDTH*1/3 - player.x < 0 thus Math.max(...,0) = 0

this means you can simply add the two to compute the total shift. and this is what your code will look like: 

int dx = 
     Math.max( GAME_WIDTH/3 - player.x, 0 ) + // too far to the left? 
     Math.min( GAME_WIDTH*2/3 - player.x, 0 ); // or too far to the right?

a little more abstraction

whatever variant you chose, it's now insanely easy to put this in a method. but let's make it a bit more meaningful than this special case. i would suggest

/**
 * Computes the distance from a point x to an interval [a,b]
 */
public static int distanceToInterval( int x, int a, int b ){
   return 
       Math.max( a - x, 0 ) + // too far to the left? 
       Math.min( b - x, 0 ); // or too far to the right? 
}

// now use as 
int dx = distanceToInterval( player.x, GAME_WIDTH/3, GAME_WIDTH*2/3 ); 
int dy = distanceToInterval( player.y, GAME_HEIGHT/3, GAME_HEIGHT*2/3 );

p.s. please note that i've replaced GAME_WIDTH - GAME_WIDTH/3 by GAME_WIDTH*2/3 throughout this post. these two give slightly different results with integer math, but i just prefer the short version as i find it more intuitive ("two thirds of the screen" vs "the entire screen and then one third back").

Upvotes: 1

Marko Topolnik
Marko Topolnik

Reputation: 200138

Unreadable, but I get your point. One thing, though: why not reuse the same check for both x and y? And by using the ternary operator you get one hack less.

static int toBounds(int max, int curr) {
  int ret;
  return ((ret = (max - max/3) - curr) < 0 || (ret = max/3 - curr) > 0)? ret : 0;
}

Upvotes: 1

Alexander Oh
Alexander Oh

Reputation: 25621

how about this?

/* calculate offset to center */
dx = X_current_center - player.x;
dy = Y_current_center - player.y;

if (abs(dx) > MARGIN || abs(dy) > MARGIN)
    /* recenter */

Upvotes: 1

Rob Wagner
Rob Wagner

Reputation: 4421

I'm not sure I understand why you have those if statements at the top at all. You should simply assign the variables when you initialize them.

dx = (GAME_WIDTH - GAME_WIDTH / 3) - player.x)
dy = (GAME_HEIGHT - GAME_HEIGHT / 3) - player.y)

They're going to be assigned to that regardless in your if statement. If it rearches the third condition then it already is zero, so there is no reason to assign it. discard the two if statements and instead stick to just straight variable assignment. Keep the condition above the for loop, that is the only check you need to make.

Upvotes: 5

Starkey
Starkey

Reputation: 9781

Your if statements are confusing to read. If you (or anyone else) need to change it at a later time, it might be very difficult and error prone.

It is a good habit to make code that not only works, but is maintainable. Since you got it to work, you should make it more maintainable.

Break the if statements in to variables that makes sense and use multiple lines. You aren't writing for a golf code contest?

You'll thank yourself later.

Upvotes: 4

Related Questions