Checking if String has unique values [closed]

Question

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;
    }
}

Answer 1

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

Answer 2

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.

Answer 3

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:

• Throw a NullPointerException up the stack
• Return false as result
• 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!

Answer 4

One more thing about loop: you can use a fast enumeration and get rid of the i variable:

 for (char c : employeeNumber.toCharArray()) {
      // ...
 }

Then, Integer.parseInt(String.valueOf(employeeNumber.charAt(i))) is not readable nor efficient.

You can replace it by:

if (Character.isDigit(c)) {
    counter += Character.getNumericalValue(c);
} else {
    // Either throw an exception or return immediately
}