# Java number formatting in accountant style

Problem

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) {
} else {
if (fCnt == 3) {
fCnt = 0;
}

fCnt++;
}

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

}
} else {
for (char a : reverse) {
if (fCnt == 3) {
fCnt = 0;
}
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;

}
``````

Solution

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 `public`but you aren’t doing any parameter validation. If the passed in `char[] ch` would be `null` you would get an `NullPointerException`.

``````    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);
}

return formatIntegerString(parts) + DECIMAL_SEPARATOR + parts;
}
``````

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);
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.

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);
}
``````