Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a small function which checks if the given String has all 0's. This method would be used quite often and want to know if it's effectively written. Any help would be greatly appreciated.

 /*
 * Input parameters: Employee Number
 * Returns true if sum of employee number is greater than 0,
 * else false.
 */
private boolean employeeNumberCheck(String employeeNumber){
    try{
        if(employeeNumber.length()==0){
            return false;
        }
        int counter=0;
        for(int i=0;i<employeeNumber.length();i++){
            counter=counter+Integer.parseInt(String.valueOf(employeeNumber.charAt(i)));
        }
        if(counter==0)
            return false;
        return true;
    } catch (Exception someException){
        logger.info("Error from method employeeNumberCheck.\n"+someException);
        return false;
    }
}
share|improve this question
up vote 5 down vote accepted

In this case I would go the regex route. Your code is basically saying: Anything that parses as a digit is fine, just so long as everything is a digit, and at least one of them is not zero.

Express that as a regular expression:

private static final Pattern validEmployeeNumber = Pattern.compile("^[0-9]*[1-9][0-9]*$");

Then your function becomes:

private static final Pattern validEmployeeNumber = Pattern.compile("^[0-9]*[1-9][0-9]*$");

private static boolean employeeNumberCheck(String employeeNumber){
    return validEmployeeNumber.matcher(employeeNumber).matches();
}   

See it running in ideone

share|improve this answer
    
Superb approach....why did I not think of this.... – Vineeth 4 hours ago
    
Beat me by 10 minutes! Great answer, though you didn't check for a possible NullPointerException that OP has caught. – TheCoffeeCup 4 hours ago

Some points:

Formatting

Your formatting is a bit hard to read. I have fixed it for you, but keep in mind of these standard Java Convention rules:

  • Space before {
  • Space after if, while, do, for etc.
  • Space surrounding operators (==, =, etc.)
  • Space after ; in for loops

Bad Practices

try {
    // code here
} catch (Exception someException) {
    // logging here
}

It is often bad practice to catch every single exception in one catch block, especially if the exceptions can be predicted. In this case, we have NullPointerException and NumberFormatException.

First, we can identify where and why these exceptions can be thrown, and if they can be prevented.

The NullPointerException can be thrown on the first line of your try block because employeeNumber can be null. You can prevent this by doing:

  1. Throw a NullPointerException up the stack
  2. Return false as result
  3. Log the error, and do one of the above

The NumberFormatException can be thrown at Integer.parseInt(). You now can just use your original approach, but catch the specific exception:

try {
    // code here
} catch (NumberFormatException someException) {
    // logging here
}

Misc

Here:

    if(counter==0)
        return false;
    return true;

You can just do:

    return counter != 0;

But most of my complaints (apart from the Exceptions) now don't really apply to the new answer that @rolfl has given. Just keep in mind of these things when writing some other code!

share|improve this answer

Redundant logic

The initial

if (employeeNumber.length() == 0) {
    return false;
}

is not needed. If the string is empty, your loop will never execute and counter will be 0 after it, so the code would return false anyway.

Performance

This code

counter = counter + Integer.parseInt(String.valueOf(employeeNumber.charAt(i)));

is a very expensive and convoluted way to check whether employeeNumer has a decimal digit at position i.

I'd use something like this instead.

final char c = employeeNumber.charAt(i);
if ((c < '0') || (c > '9')) {
    return false;
}
counter += c - '0';  // Add numerical value of the character

This works because Java's strings are Unicode and your input only allows digits anyway.

Error Handling

You should almost never do this:

catch (Exception someException) {
    …
}

What kind of exception do you expect to be thrown from within your try block? The only possibility I can see is a NumberFormatException if Integer.parseInt is given something that is not an integer representation. Any other exception you'll catch will be camouflaging some serious problem.

  • A bug in your own code, such as an indexing error in the iteration over the characters of the string.
  • Misuse by your clients, for example, passing in null as employeeNumber.
  • Serious errors in the run-time environment (unlikely in this case).

You shouldn't catch any of these.

If you omit the Integer.parseInt call as suggested above, there won't be any need for a try { … } catch { … } at all any more, which is a good thing.

Code formatting

Consider using more spaces even where they are not strictly required lexically. Most people will find

for (int i = 0; i < employeeNumber.length(); i++) {

easier to read than

for(int i=0;i<employeeNumber.length();i++){

and it is pretty much agreed upon style.

Also try to be consistent with your use of braces for single statements. Currently, you are sometimes using braces around the single statement of a control-flow construct…

if(employeeNumber.length()==0){
    return false;
}

…and sometimes you are not.

if(counter==0)
    return false;

I don't really care which style you're using but it should be consistent. From what I can tell, most Java programmers seem to prefer to always use braces, even when not strictly required.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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