Format an integer with space as thousand separator

Question

I need to format a number with space as thousand separator in my Java code for Android.

• How can these functions be improved?

  • What are the pros and cons to the first recursive method?
  • What are the pros and cons to the second substring method?

• Is there maybe an even better and easier way to solve the problem?

Java code for the recursive variant:

/**
 * Format a (int) Number with Space as Thousand Separator
 * Recursive variant
 * @param  number  the Number to Format
 * @return String  the Formatted Number
 * @since  1.0
 */
public static String intToThousandSpacedStringOS(int number) {
    boolean positive = true;
    String t = "";

    if (number < 0) {
        positive = false;
        number = -number;
    }

    if (number > 999) {
        t = intToThousandSpacedStringOS(number / 1000) + " ";
        do {
            number = number % 1000;
        } while (number > 999);
        t += String.format("%03d", number);
    } else {
        t = "" + number;
    }
    return positive ? t : "-" + t;
}

Java code for the substring variant:

/**
 * Format a (int) Number with Space as Thousand Separator
 * SubString variant
 * @param  number  the Number to Format
 * @return String  the Formatted Number
 * @since  1.0
 */
public static String intToThousandSpacedStringOS(int number) {
    boolean positive = true;
    String t;
    String r = "";
    int i = 1;

    if (number < 0) {
        positive = false;
        number = -number;
    }

    t = "" + number;
    while (t.length() > 3 * i) {
        r = t.substring(t.length() - 3 * i > 3 ? t.length() - 3 * (i + 1) : 0, t.length() - 3 * i) + " " + r;
        i++;
    }
    r += t.substring(t.length() - 3 > 0 ? t.length() - 3 : 0, t.length());

    return positive ? r : "-" + r;
}

Answer 1

I will not discuss if it's re-inventing some wheel, but comment on the two code versions given.

Recursive version

The loop

    do {
        number = number % 1000;
    } while (number > 999);

is nonsense. number % 1000 can never be > 999, so number = number % 1000; without the loop does exactly the same.

In the "loop", re-assigning a new value to the method call parameter number is considered bad style. You should introduce a new variable, e.g. int remainder = number % 1000;.

You can avoid the error-prone handling of the positive flag by utilising recursion as well:

if (number < 0) {
    return "-" + intToThousandSpacedStringOS(-number);
}

You declare your variables very early, and later change their values from an (often useless) initial value to something useful, e.g.

String t = "";

Better declare your variables in the exact same line where you know their value. Or at least, don't assign an initial value, then the compiler gives you an error message if in some execution path you forgot to assign a useful value.

To sum it up, my recursive version would read

public static String intToThousandSpacedStringOS(int number) {
    if (number < 0) {
        return "-" + intToThousandSpacedStringOS(-number);
    } else if (number > 999) {
        return intToThousandSpacedStringOS(number / 1000) + 
               String.format(" %03d", number % 1000); // note the space in the format string.
    } else {
        return String.format("%d", number);
    }
}

Iterative version

Reading and understanding this version is a challenge, and that's a red flag. Don't expect any fellow programmer (and your future self next month) to understand what a line like

r = t.substring(t.length() - 3 * i > 3 ? t.length() - 3 * (i + 1) : 0, t.length() - 3 * i) + " " + r;

does. I guess you cut a chunk out of the original decimal string and prepend that, together with a space, to the result string. Then, at least introduce two variables like

int chunkStart = t.length() - 3 * i > 3 ? t.length() - 3 * (i + 1) : 0;
int chunkEnd = t.length() - 3 * i;
r = t.substring(chunkStart, chunkEnd) + " " + r;

With the ternary-operator conditional, you probably want to avoid a negative chunkStart. I'd write

int chunkStart = Math.max(t.length() - 3 * (i + 1), 0);
int chunkEnd = t.length() - 3 * i;
r = t.substring(chunkStart, chunkEnd) + " " + r;

or

int chunkStart = t.length() - 3 * (i + 1);
if (chunkStart < 0) chunkStart = 0;
int chunkEnd = t.length() - 3 * i;
r = t.substring(chunkStart, chunkEnd) + " " + r;

You seem to love while loops. Typically, for loops are much more readable, as they combine all loop aspects in a single line - your while loop spreads its aspects over half of the method length:

• The initial value is found in line 12: int i = 1;.
• The condition is in line 20: while (t.length() > 3 * i) {.
• The updating is in line 22: i++;.

The variable i that you use in the loop is only loosely related to the values you need inside the loop. You want to iterate a position inside the string backwards from the end, in 3-character steps. A straightforward loop would be e.g.

for (int chunkEnd = t.length() - 3; chunkEnd > 0; chunkEnd -= 3) {
    ...
}