4
\$\begingroup\$

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();
}
\$\endgroup\$
  • \$\begingroup\$ Why do you replace \\. by ,, change last , and replace all , by empty string? Why you can't immediately replace . by empty string and after that replace , by . if it exists? \$\endgroup\$ – Kakadu Dec 30 '13 at 11:18
  • 5
    \$\begingroup\$ You have a couple of larger problems; Double (and floating point in general) does not store exact amounts like people expect - ie, .1 can't be represented (only something really close). This can lead to surprising results, which tends to make people itchy when it involves money. You're also storing the amount in a string, which means you have to convert it each time. For money, you want to use something based off/backed by BigDecimal, so you get the values you're expecting. \$\endgroup\$ – Clockwork-Muse Dec 30 '13 at 13:21
2
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Yes, It was in my mind to use NumberFormat. Thanks! \$\endgroup\$ – eatSleepCode Jan 2 '14 at 5:47
6
\$\begingroup\$

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

...
\$\endgroup\$
  • \$\begingroup\$ it is throwing NumberFormatException as it appends comma ,. Here comma needs to be replaced with dot .. \$\endgroup\$ – eatSleepCode Dec 30 '13 at 12:04
  • \$\begingroup\$ I have added some code after edit. any suggestion over this \$\endgroup\$ – eatSleepCode Dec 30 '13 at 12:22
  • 1
    \$\begingroup\$ @eatSleepCode No, that doesn't count: You just copied out the part of your code with least criticism. And you didn't manage without a loop, you just hid it behind a call to replaceAll. If you want to optimize for performance, you shouldn't use regexes if they aren't needed. \$\endgroup\$ – amon Dec 30 '13 at 12:58
  • 1
    \$\begingroup\$ You could remove the regexes and not use a for loop by simply changing the call to String.replaceAll() to String.replace() which takes a char argument instead of a regex String. \$\endgroup\$ – MrLore Dec 31 '13 at 18:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.