Is this a good isNaN method?

Question

public boolean isNaN(String string){

@SuppressWarnings("unused")
BigDecimal bg = null;

try{

    bg = new BigDecimal(string);

}catch(Exception e){

    return true;
}

return false;
}

There is no native isNaN method for BigDecimal. The thing I don't like about this method is the suppressed warning. I never use the variable but only the constructor to see if it doesn't throw an exception. If it does, I return the corresponding boolean.

What do you think?

Answer 1

To complement the other answer:

• I wouldn't call it isNaN as it's by far not obvious, that you mean "BigDecimal-NaN".

• Actually, "isNaN" is confusing, even when we know, it deals with BigDecimal. NaN is a special double value, which you will NOT get when you try Double.parse("foobar"). Moreover, there's no Double#isNan(String). So I'd call it "isValidBigDecimal".

• Speed could be a concern, when you plan to use it like

    if (isNan(s)) bd = new BigDecimal(s);
  

  as you parse it twice. What about tryParse returning null on failure?

---

I strongly disagree with this remark:

You should not catch Exception because it's too generic. What do you really want ?

I guess, the OP wants to know if a given string is a valid BigDecimal. If it's not, they'll get an Exception and this is a good reason to catch Exception here. When the code was long, then I'd also advise against catching "so much", but there is a single statement here, and the question is "does it succeed or not".

Note that, despite Javadoc, an NPE gets also thrown. Not catching it may be OK as nulls are a curse and it's better to disallow them everywhere, but this decision should be done explicitly.

---

I disagree with the comment

If one wants to handle both possibilities, one should write catch (NumberFormatException | NullPointerException e).

The question isNaN should answer is "Will it return a number or throw?". This makes catching Exception correct by definition. Anything else is like a search for an alternative representation of 42. The dogma "never catch Exception" should be extended by "... unless it's exactly what you need".

So, my favorite (non-optimized) solution is

public BigDecimal tryParseBigDecimal(String string) {
    if (string == null) {
        return null;
    }
    try {
        return new BigDecimal(string);
    } catch (Exception e) {
        assert e instanceof NumberFormatException || e instanceof NullPointerException : e;
        return null;
    }
}

public boolean isValidBigDecimal(String string) {
    return tryParseBigDecimal(string) != null;
}

I've added an assert expressing out expectations. This way I'd get notified if something really strange happened.

---

A much faster solution could be to use the regex

[-+]?(?:\d+\.\d*|\.\d+)(?:[eE][-+]?\d+)?

but you can imagine that it opens quite some room for bugs.

UPDATE

In the comments I was told that it is irresponsible to recommend (especially to newer programmers) that it is common to catch Exception. That's true and that's why I'm writing this. My rules are

• Be always as specific as possible. Never catch too much, especially
  • never catch a superclass when you expect a special exception
  • make you try-catch blocks as small as possible
• Always react appropriately to exceptions. Especially, never swallow them. Most of the time, declaring them in the throws clause is the best.

^{That all said, I still think that my solution is best if you need a method checking if a given String can be parsed as BigDecimal (and need no optimizations).}

Update

Actually, catching Exception is not necessary as RuntimeException would do. This covers the documented NumberFormatException, the undocumented NullPointerException and all other undocumented exceptions which may get thrown. It doesn't cover checked exception, but they can't occur (unless someone uses dirty hacks), so it's fine. I still insist that catching less is wrong.

Answer 2

You said :

I never use the variable but only the constructor to see if it doesn't throw an exception

I would ask you, then why did you declare a variable at all ? You could have done :

public boolean isNaN(final String string) {
    try {
       new BigDecimal(string);
    } catch (final Exception e) {
        return true;
    }
    return false;
}

When you don't need a variable, don't declare it.

You should not catch Exception because it's too generic. What do you really want ? The constructor throw NumberFormatException if the argument is not valid, the better option would be to catch that one instead.

Answer 3

Declaring the variable is entirely unnecessary, as has already been stated, but whether you do or not does not impact code clarity or performance, so I think it really comes down to personal preference.

The exception handling raises significantly more issues. For the most part, those have already been addressed, but I think the previous answers have skipped one major point of concern. Exceptions have their name for a reason; outside of unit tests, they should represent an exception to the normal flow of logic, and not play an active role in it.

The reason for this is that exceptions are really expensive, due to all the overhead in processing they imply. I would therefore replace the entire solution with RegEx that is as flexible as you need it to be and return whether or not it matches the input. For example:

import java.util.regex.*;

public class NumberTester
{
        private static Pattern p = Pattern.compile("-?\\d*(\\.\\d+)?");

        public static boolean test(String s)
        {
                return p.matcher(s).matches();
        }
}

The above is a basic example, so more work will have to go into the expression if you want to match something like scientific notation, or numbers with commas, but unless your actual goal is to write canBeParsedByBigDecimal(String s), I think a solution like this is significantly better.