Odd behavior of a float method. Overloading?

Question

Does anyone see a reason why this would simply return nothing?

public static float  sumDigits(int n) {
    float retVar = 0;
    while (n > 10) {
        float d;
        for (d = 0; d < n; d *= 10);
        d /= 10;
        retVar += n / d;
        n %= d;
    }
    n += n;
    return retVar;
}

I called it via

System.out.println(sumDigits(123) + "");

Answer 1

I actually think I see what your for loop is supposed to do, it's a little pow function. But as some others have pointed out, d starts out as 0 so it always ends up 0.

Just changing d to 1 will stop your infinite loop:

for (d = 1; d < n; d *= 10);

But the answer is still wrong since it looks like you're trying to sum the digits. (e.g. 1 + 2 + 3 = 6, but instead the answer returned is 3.53)

Since you are dealing with the individual digits, using float is bad here because you don't want fractional values. You want the digits to round. So you need to change your temp variables to int.

There's one last thing which is the last digit is skipped. At this point I get 3 as the answer. After changing while(n > 10) to while(n > 0), the right answer is returned.

So the code would be this:

public static int sumDigits(int n) {
    int retVar = 0;
    while (n > 0) {
        int d;
        for (d = 1; d < n; d *= 10);

        d /= 10;
        retVar += n / d;
        n %= d;
    }
    return retVar;
}

As a tip, there's a simpler way to solve this same problem which is to start with the smallest digit instead of the largest:

while(n > 0) {
    retVar += n % 10;
    n /= 10;
}

Final Note

If I'm right about the for loop, the important thing to take home is that if you write stuff like this almost nobody will think it's intentional. Syntax like this might actually work correctly but it's obscure and more like a novelty.

The loop can be replaced like this which is much more clear and just as compact:

int d = 0;
while(d < n) {
    d *= 10;
}

Or if you really want:

int d = 0;
while(d < n) d *= 10; // not recommended but clearly has a body

Answer 2

Bad semicolon placement:

for (d = 0; d < n; d *= 10); // that terminal semicolon will mess you up!

This is equivalent to:

for (d = 0; d < n; d *= 10) {
  // do nothing
} 

• Get rid of that terminal semicolon.
• Make your d variable local to the for loop: for (int d = 0; d < n; d *= 10) {...}
• Always enclose all loop blocks in curly braces
• Good code formatting isn't there just to make your code loop pretty -- it also helps you debug.

Answer 3

As has been pointed out, your semicolon placement means you're executing a loop that does nothing:

    for (d = 0; d < n; d *= 10);
    d /= 10;
    retVar += n / d;
    n %= d;

But just fixing the loop structure won't help you:

    for (d = 0; d < n; d *= 10) {
        d /= 10;
        retVar += n / d;
        n %= d;
    }

First of all, if you start with d = 0, both d /= 10 and d *= 10 will leave d equal to 0. That can't be what you want. This should result in an infinite loop.

Second, assuming you start with some other d: First you divide it by 10, in the first statement in the loop. Then, when you go back up to repeat the loop, you multiply it by 10 again. That just takes you back where you've started. So that also results in an infinite loop.

In general, when you have a variable in a for loop that you are varying, it's wrong to also modify the same variable in the body of the loop. (d in this example.) Usually this is just a bug. Even if it's not a bug and you know what you're doing, I think it's still bad practice because it's very confusing for the reader; if you really want to do something like that, use a while loop instead.