Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

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

Requirement

Validate two inputs (from and to). Input can be either Number or String (value hast to be date in UTC format) "from" and "to" both cannot be empty or null but one of them can be empty or null.

Pre condition : Use only Java 7

My simplest validation logic. Please let me know if I can optimize this code. Ignore JSONException part.

private static void validate(Object from, Object to) throws JSONException {
    if ((from == null && to == null)
            || (isEmpty(from) && isEmpty(to))) {
        throw new JSONException("\"from\" or \"to\" has to be specified in range filter");
    }
    if (!isEmpty(from)) {
        validateRange(from);
    }
    if (!isEmpty(to)) {
        validateRange(to);
    }
}

private static void validateRange(Object range) throws JSONException {
    //Valid date
    if (range instanceof String) {
        String input = (String) range;
        //Empty check 
        if (!input.matches("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$")) {
            throw new JSONException("range check field is not a valid date");
        }
    }
}
private static boolean isEmpty(Object range) throws JSONException{
    //Instance check
    if (!(range instanceof Number) || !(range instanceof String)) {
        throw new JSONException("From and To in range can be only of type Date or Integer");
    }

    if (range instanceof String) {
        return ((String) range).trim().isEmpty();
    } else {
        return true; //For Number this check always true.
    }
}
share|improve this question
    
Please do not add, remove, or edit code in a question after you've received an answer. The site policy is explained in What to do when someone answers. – Mast Apr 4 at 21:26
    
Yes true. I was modifying the question based on my unit tests not based on the answer provided below. There were logic flows which I found while running through my unit tests – Raghu K Nair Apr 4 at 21:27
    
I'd recommend taking both the advice of the answer and the found flaws to write a new version of your code and post this as a new question. This is called a follow-up. Feel free to link back to this question for bonus context. While you're at it, please take a look at how-to-ask to make your next question an above-average one. Those tend to get more exposure, more upvotes and more/better answers. – Mast Apr 4 at 21:37
1  
Thanks Mast . I will go through the link and improve the questions for the next post. – Raghu K Nair Apr 4 at 21:45

Possible bug

if (!(range instanceof Number) || !(range instanceof String)) {
    throw new JSONException("From and To in range can be only of type Date or Integer");
}

Does it even work?? It throws an exception if range is either not a Number, or not a String. But it's inevitable, because if it's a String, then it can't be a Number, and vice versa. Didn't you mean &&?

Communicating validation failure

Why does validation failure result in a JSONException (of all exceptions)? I can't see how range object not being a Number is a JSON issue?

Besides, using exceptions to communicate an error isn't something I would choose to do lightly. Exceptions are quite costly - they're quite a heavy-weight mechanism which rates very poorly as far as performance is concerned, plus it forces consuming code to wrap validation calls in clunky try-catch blocks even if all it wanted was just to check whether the value is correct.

A better alternative would be to return a bool, or some sort of an error object (containing additional info about the input).

Contextual info

Furthermore, your exceptions aren't informative. They only tell me what criteria the value failed to meet, but not why. "range check field is not a valid date" - what is it, then? Understanding a possible bug or malfunction is much easier if the actual, unexpected value is quoted in error logs.

if (!(range instanceof Number) || !(range instanceof String)) {
    throw new JSONException("From and To in range can be only of type Date or Integer");
}

Apart from the condition looking doubtful to me (as explained in the first paragraph), this exception message is not true - your input is checked for being of String type, not Date type.

Code style

if (range instanceof String) {
    return ((String) range).trim().isEmpty();
} else {
    return true;
}

else after a return is redundant. This whole expression could be simplified to:

return !(range instanceof String) || ((String) range).trim().isEmpty();

or at least

return range instanceof String
    ? ((String) range).trim().isEmpty()
    : true;

Note that depending on the platform and libraries you are using, the trim().isEmpty() bit could possibly be replaced by an utility method call. See http://stackoverflow.com/questions/5819800/whats-the-better-way-to-check-if-a-string-is-empty-than-using-string-trim-len for suggestions (although they may not apply in your case).

Comments

//Instance check
if (!(range instanceof Number) || !(range instanceof String)) {

I suggest that you get rid of this "instance check" comment, as it explains the obvious and doesn't add any value to your code.

//Empty check 
if (!input.matches("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$")) {

And this comment is simply untrue. The condition doesn't only fail if input is empty - it fails when it doesn't match your predefined data format.

Naming

private static void validateRange(Object range)

It doesn't validate the whole range (comprised of from and to). It only validates from or to on its own, which is called a bound.

private static boolean isEmpty(Object range) throws JSONException

I can accept a validate or assert method to throw an exception, but this one is called isEmpty - this name indicates it's a method I can use to calmly obtain some info about the object. And yet it performs both responsibilities at once - apart from providing info, it also validates the input in an aggressive manner (throwing an exception). This violates the principle of least surprise in my opinion.

share|improve this answer
1  
Thanks for the answer . I agree with some of the points . – Raghu K Nair Apr 4 at 20:10

Here is what my implementation. I agree with Konrad regarding the throwing a error message instead of constructing exception everywhere but I choose to not do it mainly due to code clarity.

   private static void validate(Object from, Object to) throws JSONException {
    instanceCheck(to);
    instanceCheck(from);

    if ((from == null && to == null)
            || (isEmpty(from) && isEmpty(to))
            || (from == null && isEmpty(to))
            || (to == null && isEmpty(from))) {
        throw new JSONException("\"from\" or \"to\" has to be specified in range filter");
    }
    if (!isEmpty(from)) {
        validateDate(from);
    }
    if (!isEmpty(to)) {
        validateDate(to);
    }
}

private static void instanceCheck(Object range) throws JSONException {
    if (range != null
            && !(range instanceof Number)
            && !(range instanceof String)) {
        throw new JSONException("\"from\" and \"to\" in range can be only of type Date or Number");
    }
}

private static boolean isEmpty(Object range) throws JSONException {

    return (range instanceof String)
            ? ((String) range).trim().isEmpty() : Boolean.FALSE;

}

private static void validateDate(Object date) throws JSONException {
    //Valid date
    if (date instanceof String) {
        String input = (String) date;
        //Empty check 
        if (!input.matches("^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$")) {
            throw new JSONException("Not a valid date");
        }
    }
}
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.