current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite
1

I am building a infinite vertical platformer for mobile platforms using Unity3D. I am using the accelerometer to move the character left and right on the screen.

if (Input.acceleration.y > -0.2f && Input.acceleration.y < 0.2f) {
                        maxSpeed = 17;
                }

        if (Input.acceleration.y > -0.5f && Input.acceleration.y < -0.2f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > 0.2f && Input.acceleration.y < 0.5f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > -0.7f && Input.acceleration.y < -0.5f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y > 0.5f && Input.acceleration.y < 0.9f) {
            maxSpeed = 25;
        }
        if (Input.acceleration.y < -0.9f) {
            maxSpeed = 40;
        }
        if (Input.acceleration.y  > 0.9f) {
            maxSpeed = 40;
        }

Basically the farther the device is tilted the faster the character moves. I'm pretty sure there is a way better way to write this code but I'm pretty terrible at math and I can't figure it out. Any suggestions on cleaning this up a bit?

share|improve this question

4 Answers 4

up vote 7 down vote accepted

I know absolutely nothing about or , so if there's a language specific assist, I have no idea about it!

I notice each condition checks Input.acceleration.y, so that's one bit of information that repeats. I also notice we have three speeds, yet we have seven conditionals!

First of all, the negatives are confusing. Find the absolute value first.

I'm going to assume a number followed by an "f" means float. So maybe we need something like:

float acceleration = Math.Abs(Input.acceleration.y);

Great, now it's going to be easier to handle this!

Now all we need is a simple check like so:

if (acceleration < 0.2f) {
    maxSpeed = 17;
} else if (acceleration < 0.9f) {
    maxSpeed = 25;
} else {
    maxSpeed = 40;
}

Your original numbers were a little weird, but if you meant for such a strange pattern, correct me!

share|improve this answer
    
This looks quite nice, but won't get the same result for e.g -0.2f –  Heslacher yesterday
    
What do you mean? If the acceleration is -0.2f, then find the absolute value: 0.2f. Plug this in and you get a maxSpeed of 25. In the code, it actually omits -0.2f, which is what I meant by "strange pattern"! –  Alex L yesterday
    
In your code, you actually omit -0.2f-> not me but the OP –  Heslacher yesterday
    
Sorry @Heslacher, I was thinking you were the OP replying! I've been up for a while ;) –  Alex L yesterday
    
no problem at all –  Heslacher yesterday
up vote 6 down vote

Here is your number line:

  …<-40->|      |<-25->|<--25-->|<-17->|<-17->|<--25-->|<----25----->|<-40->…
        -0.9  -0.7   -0.5     -0.2     0    +0.2     +0.5          +0.9

The first likely bug that I see is that the ranges are discontinuous. For example, if Input.acceleration.y is exactly 0.5, then none of the cases applies.

Once you fix that bug, you can simplify the number line to the following equivalent:

  …<-40->|      |<------25----->|<-----17---->|<---------25--------->|<-40->…
       -0.9   -0.7            -0.2          +0.2                   +0.9

The next likely bug is that the two sides are asymmetrical, and there is a strange gap between -0.9 and -0.7. Perhaps you meant to fill that in with maxSpeed = 25? I'll assume you meant…

  …<-40->|<---------25--------->|<-----17---->|<---------25--------->|<-40->…
       -0.9                   -0.2          +0.2                   +0.9

Since the entire number line from -∞ to +∞ is covered, you should be able to avoid the double-ended inequality comparisons, and instead chain them with else-ifs. Or, if you like more compact expressions, you could use ternary operators:

float absY = Input.acceleration.y < 0 ? -Input.acceleration.y : Input.acceleration.y;
maxSpeed = (absY < 0.2f) ? 17
         : (absY < 0.9f) ? 25
         :                 40;

The first line could also be written using Math.Abs(), but I chose to write it this way to avoid converting the float to a double.

share|improve this answer
up vote 4 down vote

You're missing some cases, for example what should happen when y = 0.5? Or -0.9f < y < -0.7?

I think this is valid representation of your current code:

y:   -inf    -0.9f  -0.7f  -0.5f   -0.2f    0.2f    0.5f    0.9f       +inf
m:        40   ?   ?  ?  25  ?   25  ?   17  ?   25  ?   25  ?    40

Based on this diagram, the code posted in @AlexL's answer is likely what you want to achieve. If you're trying to do something else, please post another question with more details :)

share|improve this answer
up vote 4 down vote

Also I favourite Alex L's answer, her is an alternative way

The first 5 if conditions are checking if the given Input.acceleration.y is between two float values.

So we can extract the between part to an extension method like

public static Boolean Between(this float num, float lower, float upper)
{
    return lower < num && num < upper;
}

we assign the value of Input.acceleration.y to a variable, first for readability and second this property only needs to be read only once.

float accelerationY = Input.acceleration.y;

next we are using the Between() method

    if (accelerationY.Between(-0.2f, 0.2f))
    {
        maxSpeed = 17;
    }
    else if (accelerationY.Between(-0.5f, -0.2f)
        || accelerationY.Between(0.2f, 0.5f)
        || accelerationY.Between(-0.7f, -0.5f)
        || accelerationY.Between(0.5f, 0.9f))
    {
        maxSpeed = 25;
    }
    else if (accelerationY < -0.9f || accelerationY>0.9f)
    {
        maxSpeed = 40;
    }
share|improve this answer
2  
If you want to preserve the original behaviour (in a bug-compatible way), this is the way to go. –  200_success♦ yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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