Optimize parsing of number for currency conversion

Question

I have written this working block, but is it optimized? I am getting currency in this format: 1.234,25.

public static String convertCurrency(String value, String exchangeCurrencyId)
{
    java.text.NumberFormat format = null;
    value = value.replaceAll("\\.", ",");
    char[] chars = value.toCharArray();
    chars[value.lastIndexOf(',')] = '.';
    value = new String(chars);
    value = value.replaceAll(",", "");
    Double price = Double.valueOf(value);
    price = price * (50); //exchange rate

    return price.toString();
}

Answer 1

It seems like you tried to use NumberFormat but gave up on the idea. You should take advantage of it — after all, that's what the standard library is there for.

Furthermore, I would split the parsing and the currency conversion into two separate functions.

You should avoid "stringly typed" variables. Once you parse the string into a number, hang on to the number, not the string. Convert back into string form as late as possible, just for presentation purposes.

import java.math.BigDecimal;
import java.text.DecimalFormat;
import java.text.NumberFormat;
import java.text.ParseException;
import java.util.Locale;

public class CR38317 {
    public static BigDecimal parseCurrency(String value) throws ParseException {
        // Pick a suitable locale.  GERMANY, for example, uses the 1.234,56 format.
        // You could call .getCurrencyInstance() instead, but then it will
        // require the value to contain the correct currency symbol at a
        // specific position.
        NumberFormat fmt = NumberFormat.getNumberInstance(Locale.GERMANY);

        // By default, fmt.parse() returns a Number that is a Double.
        // However, some decimals, such as 0.10, cannot be represented
        // exactly in floating point, so it is considered best practice to
        // use BigDecimal for storing monetary values.
        ((DecimalFormat)fmt).setParseBigDecimal(true);

        return (BigDecimal)fmt.parse(value);
    }

    public static BigDecimal convertCurrency(BigDecimal inValue, String inCurrency, String outCurrency) {
        // TODO: Derive the conversion rate from the currencies
        return inValue.multiply(new BigDecimal(50));
    }

    public static void main(String[] args) throws ParseException {
        BigDecimal in = parseCurrency("1.234,56");
        BigDecimal out = convertCurrency(in, "EUR", "INR");
        System.out.println("In = " + in);
        System.out.println("Out = " + out);
    }
}

Answer 2

This code has various issues. Here is what I understand your code does:

The code takes a formatted number which consists of digits, separators (commas or periods). The last separator is used as the decimal separator, all other separators are deleted.

Then it multiplies the number with some exchange rate, and returns that as a string.

The issues with your code are:

• format is not used.
• exchangeCurrencyID is not used.
• The exchange rate is hard-coded
• You use regexes for simple transliterations
• Your code obfuscates the intent.
• Your method does multiple unrelated things:
  1. Sanitizing and parsing the number string
  2. Applying exchange rate conversion These separate responsibilities should be performed by two separate methods.

If we wish to avoid using regexes for a task that does not need them, we could write something like this:

StringBuilder sanitized = new StringBuilder();
boolean decimalSeparatorFound = false;
char[] chars = value.toCharArray();
for (int i = chars.length-1; i >= 0; i--) {
    if (chars[i] == ',' || chars[i] == '.') {
        if (decimalSeparatorFound) continue;  // skip this char
        decimalSeparatorFound = true;
        sanitized.append('.');
    } else {
        sanitized.append(chars[i]);
    }
}
double price = Double.valueOf(sanitized.reverse().toString());

...