4
\$\begingroup\$

For a class, I need to ask the user for a double. To verify that the number can be safely passed to Double.parseDouble, I wrote the following function. I'd like to have it heavily scrutinized; including the comments.

  • Are my comments helpful? They're more directed towards the marker than a maintainer down the road. Should I phrase them this way if it's for a class, and not "real code" that will need to be maintained later?

  • Is my assertion about the nature of charAt correct? Should I be caching str.charAt(i) in the loop?

  • I was rather proud of the first validity check, but found it to be kind of ugly when I looked at it the next day. Is there any better way that it could be written?

  • Anything else that could be improved. Be critical please!

As @h.j.k pointed out, this would be entirely easier by just catching the exception that Double.parseDouble throws, but there's no fun in that! I wanted the exercise.

    import static java.lang.Character.*;

    public static boolean strIsNumeric(String str) {
        //If the string is empty, or if it isn't a '-' or digit, return false.
        //We need a check like this or a prefixed '-' will prevent it from
        // being recognized as a negative number.
        //This is safe because logical-OR "short-circuits", the right hand will only
        // evaluate if the left is false. If this wasn't the case, the calls to "charAt(0)"
        // would cause an exception on empty String.
        if ( str.isEmpty() || (str.charAt(0) != '-' && !isDigit(str.charAt(0))) ) {
            return false;
        }

        //We're skipping the first char since it was checked above.
        boolean foundADecimal = false;
        for (int i = 1; i < str.length(); i++) {
            //To avoid repeated evaluations
            char c = str.charAt(i);

            //Check to see if there's at most 1 decimal
            if ( !foundADecimal && c == '.' ) {
                foundADecimal = true;

                //We've "verified" this char already as a valid decimal point,
                // so we need to skip the next "isDigit" check.
                continue;
            }

            if ( !isDigit(c) ) {
                return false;
            }
        }

        return true;
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ Any reason why you'll not prefer to catch NumberFormatException and then... do something about it? Or is this an academic exercise? \$\endgroup\$ – h.j.k. May 20 '15 at 23:36
  • \$\begingroup\$ @h.j.k. I tend to lean towards pre-emptive checks for the exercise. Ya, just catching the exception would be loads easier and more efficient, but there's no fun in that. Plus, I went through a "functional-phase" which seems leans away from exceptions. I'll add a note, thanks. \$\endgroup\$ – Carcigenicate May 20 '15 at 23:41
  • \$\begingroup\$ all rights, cool. :) \$\endgroup\$ – h.j.k. May 20 '15 at 23:44
  • 1
    \$\begingroup\$ @h.j.k. Note that catching the exception is rather slow in Java. An optimized way can look like this. \$\endgroup\$ – maaartinus May 21 '15 at 0:00
6
\$\begingroup\$

There are three validation failures (ignoring case sensitivity) that Double.parseDouble() would have accepted otherwise:

As for your comments... they should explain the why, not the how. The comments you have now are only explaining the how, so you should consider removing them when you get the chance to write code outside of academic exercises in the future...

Variable names... it depends. i is quite well-understood, but some may frown upon a single c to represent a character, or even str for your method argument... I'm not suggesting to be overly descriptive (currentCharacterOfMethodArgument is an example), but maybe an actual English word (current?) might be slightly more readable for your marker.

And just for completeness, remember to do a null check at the start too, before str.isEmpty(). :)

edit And since you asked about it, the implementation of charAt(int) is just:

public char charAt(int index) {
    if ((index < 0) || (index >= value.length)) {
        throw new StringIndexOutOfBoundsException(index);
    }
    return value[index];
}

The only 'evaluation' here is the if statement for out-of-bounds check, which I don't think is that significant/performance-hurting (if you're alluding to this indirectly as well). It's fine both ways whether you want to 'cache' it first.

\$\endgroup\$
  • \$\begingroup\$ Note that there's another range check in the array access, but when you're lucky, both get optimized away. I guess they always do. \$\endgroup\$ – maaartinus May 21 '15 at 0:09
  • \$\begingroup\$ Thank you h.j.k. I didn't consider your first cases. I'll see if I can neatly insert something to deal with the second case; I'd say the other 2 are beyond the scope of the assignment (although, if I ever needed a function like this for broader use, it would be nice to have it already working). And as I mentioned in the question, my comments are specifically aimed at the marker, not a maintainer. For an assignment, is this inappropriate? Should I just pretend that this is code that will actually be used? And is a null check really necessary? \$\endgroup\$ – Carcigenicate May 21 '15 at 14:30
  • \$\begingroup\$ (Ran out of characters). Theoretically, any argument passed in to any function could be null. I don't think it would be correct to check literally every argument in every function since from what I've read, passing nulls to represent failure is frowned upon, and becoming less common. \$\endgroup\$ – Carcigenicate May 21 '15 at 14:31
  • \$\begingroup\$ @Carcigenicate the null check really depends on what your method is expected to do with them. There are cases where it's enough to just throw the NullPointerException. There are cases where a default value should be used in place. There are also cases where a null passed into a method indicates something is very wrong with the values given by the caller, which necessitates ERROR logging and some kind of specific exception handling must be done as a result. Let's hope callers of your method knows how to recover from passing a null to it then. ;) \$\endgroup\$ – h.j.k. May 21 '15 at 14:59
  • \$\begingroup\$ @Carcigenicate for an assignment, it depends if one will be marked down for an arbitrary "little/no comments" rule. :p It may be possible that students are given higher marks when they can describe (at least initially) how their code works, but YMMV. \$\endgroup\$ – h.j.k. May 21 '15 at 18:05

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.