12
\$\begingroup\$
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?

\$\endgroup\$
  • 3
    \$\begingroup\$ You're using exceptions to control application flow. This may not be avoidable but its not ideal. Does BigDecimal or any other parsing class offer a mechanism that doesn't throw exceptions? Perhaps use Apache NumberUtils isNumber: commons.apache.org/proper/commons-lang/javadocs/api-2.6/org/… . If you must use your own implementation peek at the source for isNumber. \$\endgroup\$ – Freiheit Oct 8 '14 at 13:35
  • \$\begingroup\$ I personally would rather test for positive vs negative. IsANumber vs IsNotANumber. \$\endgroup\$ – The Muffin Man Oct 8 '14 at 21:14
  • 2
    \$\begingroup\$ @TheMuffinMan Although I agree that the resulting double negatives (like !isNaN()) are annoying and require you to think for a second, isNaN is "standard" in several programming languages. \$\endgroup\$ – IQAndreas Oct 9 '14 at 20:15
27
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I would agree with you that managing 'NullPointerException' should be considered (which I did not). Your answer made me aware that there is a "special" value for NaN in Java. I think the op want a more general definition of NaN to what the abbreviation stand for, check if the input is a number or not. In fact, that was my interpretation of the question, I could be wrong. \$\endgroup\$ – Marc-Andre Oct 7 '14 at 21:48
  • 6
    \$\begingroup\$ If one wants to handle both possibilities, one should write catch (NumberFormatException | NullPointerException e). Even better is to simply do if (string != null) before trying to create a BigDecimal. Catching all exceptions is convenient but it's almost never the right way to do things, because it obscures programmer mistakes. \$\endgroup\$ – VGR Oct 8 '14 at 1:39
  • \$\begingroup\$ @VGR I still mostly disagree, see my edit. Concerning obscuring if mistakes... yes, but I can't imagine any place leaving less place for mistakes. And guarding against mistakes in the standard library makes here no sense, as what you want to test is what the standard library does - i.e., my catch clause is tautologically correct. \$\endgroup\$ – maaartinus Oct 8 '14 at 2:39
  • \$\begingroup\$ Hm, coming from C#, I was going to suggest stack overflow and out of memory as two exceptions it wouldn't be appropriate to catch here. But now I realise that in Java these don't derive from Exception. \$\endgroup\$ – Ben Aaronson Oct 8 '14 at 16:58
  • 1
    \$\begingroup\$ @maaartinus I think it is irresponsible to recommend (especially to newer programmers) that it is common to 'catch Exception'. This is not a best practice, it is rarely appropriate and is a bad short-cut to encourage. NumberFormatException and NullPointerExeptn do the trick here just fine. If any other exceptions ever get thrown it would be prudent to evaluate the implications of them and how to handle them at that time. I believe that your assert statement is reducing readability for the same outcome, and when your assert fires we may never know what exception caused it in order to fix it! \$\endgroup\$ – ultimate_guy Oct 8 '14 at 18:29
27
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Seems I got a little lightheaded there. Thanks. \$\endgroup\$ – enon Oct 7 '14 at 20:03
  • \$\begingroup\$ I'll try to have a nicer tone, but old habits die hard. Constructor are rarely use without a variable so it's easy to have an un-used variable. \$\endgroup\$ – Marc-Andre Oct 7 '14 at 20:06
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Oops; just saw the RegEx footnote on @maaartinus's answer. I guess I'll leave this here anyway so the point is justified, since he didn't go into why the current method is slower. \$\endgroup\$ – mirichan Oct 8 '14 at 8:18

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.