Java number formatting in accountant style

Question

I have written a string number formatter. I would like to know if this can be improved and any suggestions with how it's done.

The purpose of this function is to convert a double that has been converted to a string, using String.valueOf to a accounting style formatted number.

/**
     * this function formats a number string to show commas and an arbitrary amount
     * of decimal places
     * @param string the number String
     * @param decimal the decimal precision
     * @return 
     */
private String formatNumberString(String string, int decimal) {

    char[] num = string.toCharArray();
    char[] reverse = reverse(num);
    int cnt = 0;
    List<Character> chars = new ArrayList();
    boolean fd = false;
    int fCnt = 0;

    if (contains(reverse, '.')) {
        for (char a : reverse) {
            if (!fd) {
                chars.add(a);
            } else {
                if (fCnt == 3) {
                    chars.add(',');
                    fCnt = 0;
                }
                chars.add(a);

                fCnt++;
            }

            if (a == '.') {
                fd = true;
            }

        }
    } else {
        for (char a : reverse) {
            if (fCnt == 3) {
                chars.add(',');
                fCnt = 0;
            }
            chars.add(a);
            fCnt++;
        }

    }
    char[] fin = new char[chars.size()];
    int cntr = 0;
    for (Character ch : chars) {
        fin[cntr++] = ch;
    }

    String finall = String.valueOf(reverse(fin));
    if (decimal == 0) {
        int dot = finall.indexOf(".");
        finall = finall.substring(0, dot);
    }

    if (finall.equals("0") || finall.equals("0.0") || finall.equals("0.00")) {
        finall = " - ";
    }

    return finall;
}

My contains function checks to see the array contains a decimal

/**
     * this function checks to see if a decimal is present in char array
     * @param ch
     * @param c
     * @return 
     */
    public boolean contains(char[] ch, char c) {
        for (char a : ch) {
            if (a == c) {
                return true;
            }
        }
        return false;
    }

My reverse function is simple it reverses the char array

 public char[] reverse(char[] ch) {
        char[] temp = new char[ch.length];
        int cnt = 0;
        for (int i = ch.length - 1; i >= 0; i--) {
            temp[cnt++] = ch[i];
        }
        return temp;

    }

Answer 1

If you come back in a few months to that code you will likely have a problem at figuring out what your code is doing. This is due to the abbreviations you used to name your things like

int cnt = 0;

boolean fd = false;
int fCnt = 0;  

for e.g you just won't know what fd should represent.

Naming things is very improtant in the programming field on the other side it is pretty hard as well. Try to give your things meaningful names stating its purpose. By reading the name later you will grasp easier what your code is about.

---

char[] fin = new char[chars.size()];
int cntr = 0;
for (Character ch : chars) {
    fin[cntr++] = ch;
}  

It would be better to just use the toArray() method specified by the List<E>.

---

For reversing the array you could use ArrayUtils.reverse() see also https://stackoverflow.com/a/2138004/2655508 which lives in Commons.Lang

---

You contains() and reverse() method are publicbut you aren't doing any parameter validation. If the passed in char[] ch would be null you would get an NullPointerException.

Answer 2

    List<Character> chars = new ArrayList();

So you want to produce a string by repeatedly appending characters to it. Then you want to reverse the string. Java has a type that does exactly that: StringBuilder. StringBuilder has both append and reverse as operations and is designed specifically to do what you are doing.

    if (finall.equals("0") || finall.equals("0.0") || finall.equals("0.00")) {
        finall = " - ";
    }

Why wait to do this until after you process the number? If any of these are true, you don't need to add any thousands separators.

    int cnt = 0;

You never use this.

You also have fCnt and cntr, which you use sequentially (i.e. you stop using fCnt before you start using cntr). Such name conflicts are often a sign that you should break up a method into smaller methods.

Another sign that you should break this method into smaller methods is the two different behaviors: with a decimal point and without.

Consider

    private static String formatNumberString(String number, int precision) {
        if (number.matches("0([" + DECIMAL_SEPARATOR + "]0*)?")) {
            return " - ";
        }

        String [] parts = number.split(Pattern.quote(DECIMAL_SEPARATOR));

        if (parts.length > 2) {
            throw new IllegalArgumentException("Numbers can only have one decimal point.");
        }

        if (parts.length == 1 || precision == 0) {
            return formatIntegerString(parts[0]);
        }

        return formatIntegerString(parts[0]) + DECIMAL_SEPARATOR + parts[1];
    }

Now we don't have any repeated code. Instead, we call the same method in two different ways.

Note that this uses a constant instead of the magic value ..

This also checks for extra decimal separators.

If the number is any variant of zero, it gets replaced with a hyphen.

This is declared static because it doesn't make use of any object fields. All state is in the parameters.

    private static String formatIntegerString(String number) {
        if (number == null || number.length() == 0) {
            return number;
        }

        char [] digits = new StringBuilder(number).reverse().toString().toCharArray();
        StringBuilder separated = new StringBuilder();

        separated.append(digits[0]);
        for (int i = 1; i < digits.length; i++) {
            if (i % 3 == 0) {
                separated.append(THOUSANDS_SEPARATOR);
            }

            separated.append(digits[i]);
        }

        return separated.reverse().toString();
    }

This is the meat of the original method. Since we already stripped off any decimal separators, we don't have to look for them here. In fact, the logic here is the same whether the original number had a decimal separator or not.

Rather than creating manual method for reverse, this uses the built-in version.

Not to argue with the idea of using a completely built-in solution, but if I were doing something like this, this is more what I would do.

Answer 3

This approach is complicated, buggy, and in my opinion the wrong way to go about it altogether.

The JavaDoc claims that decimal parameter lets you specify the precision, but that's a lie. The only place where you ever use decimal is in

if (decimal == 0) {
    int dot = finall.indexOf(".");
    finall = finall.substring(0, dot);
}

… so it's really just a boolean switch. Your formatter doesn't do rounding or zero-padding as I would expect.

Checking for three specific representations of zero is weird, and also wrong, if the input string has more zeroes:

if (finall.equals("0") || finall.equals("0.0") || finall.equals("0.00")) {
    finall = " - ";
}

Suggested solution

Use the standard Java number-formatting tools: parse the input as a BigDecimal, then use a DecimalFormat.

private static String formatNumberString(String s, int decimal) {
    BigDecimal num = new BigDecimal(s);

    // BigDecimal.equals() would require the precision to match as well
    if (0 == BigDecimal.ZERO.compareTo(num)) return " - ";

    DecimalFormat fmt = new DecimalFormat("#,##0.0;(#,##0.0)");
    fmt.setMaximumFractionDigits(decimal);
    fmt.setMinimumFractionDigits(decimal);
    return fmt.format(num);
}