current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I would like a review of how I am comparing two java.util.String in a private method in a java.util.Comparator. Either of the Strings could be null or blank, and would be "less than" the other String if that other String were not null/blank.

My gut feeling is that this is at the very least inelegant, probably difficult to read, and at worst inefficient if it had to be done millions of times per second. Oh, and there could even be a flaw in the logic!

Is there a better way to do this?

private Integer compareDateStrings(BeanToDoTask arg0, BeanToDoTask arg1, String strProperty) {

    /* Don't worry too much about this part. */
    String strDate0 = BeanUtils.getProperty(arg0, strProperty); _logger.debug("strDate0 = " + strDate0);
    String strDate1 = BeanUtils.getProperty(arg1, strProperty); _logger.debug("strDate1 = " + strDate1);
    /* If strDate0 is null or blank and strDate1 is not, then strDate1 is greater. */
    if ((strDate0 == null || strDate0.equals(""))) {
        if (strDate1 != null && !strDate1.equals("")) {
            return -1;
        } else {
            /* They both are null or blank! */
            return 0;
        }
    }
    /* We know strDate0 is not null or blank. */ 
    if (strDate1 == null || strDate1.equals("")) {
        return 1;
    }
    /* At this point neither strDate0 or strDate1 are null or blank, so let's compare them. */
    return strDate0.compareTo(strDate1);
}
share|improve this question

3 Answers 3

up vote 3 down vote accepted

You could write the code like this, it is doing the same, but I think it is more readable, you almost don't need any comment, to assume the return value.

private Integer compareDateStrings(BeanToDoTask arg0, BeanToDoTask arg1, String strProperty) {
    String strDate0 = BeanUtils.getProperty(arg0, strProperty);_logger.debug("strDate0 = " + strDate0);
    String strDate1 = BeanUtils.getProperty(arg1, strProperty);_logger.debug("strDate1 = " + strDate1);
    return compareDateStrings(strDate0, strDate1);
}

private Integer compareDateStrings(String strDate0, String strDate1) {
    int cmp = 0;
    if (isEmpty(strDate0)) {
        if (isNotEmpty(strDate1)) {
            cmp = -1;
        } else {
            cmp = 0;
        }
    } else if (isEmpty(strDate1)) {
        cmp = 1;
    } else {
        cmp = strDate0.compareTo(strDate1);
    }
    return cmp;
}

private boolean isEmpty(String str) {
    return str == null || str.isEmpty();
}
private boolean isNotEmpty(String str) {
    return !isEmpty(str);
}
share|improve this answer
2  
This goes along the lines of what I would've done too. First of all, it's odd to have a method called compareDateStrings that doesn't actually take those date strings as its parameters. Then, the a == null || a.equals("") is a repeating pattern that should be refactored into a more readable method of its own like @burna did here. I don't like the isNotEmpty method though, I'd just use !isEmpty(...). Finally, you might want to take a look at this thread: stackoverflow.com/questions/481813/… –  ZeroOne Jan 6 '13 at 0:45
    
@burna: That is indeed much more readable. We aren't supposed to say "Thanks" in Stack Exchange, so I'll just say "Cheers". –  John Fitzpatrick Jan 6 '13 at 20:21
    
I suggest at least to avoid the isNotEmpty method. You could just switch the if and else case and continue using the isEmpty method. –  tb- Jan 8 '13 at 11:32
up vote 3 down vote

I would use boolean variables to make the code more readable:

private int compareDateStrings(BeanToDoTask arg0, BeanToDoTask arg1, String strProperty) {
    /* Don't worry too much about this part. */
    String strDate0 = BeanUtils.getProperty(arg0, strProperty); _logger.debug("strDate0 = " + strDate0);
    String strDate1 = BeanUtils.getProperty(arg1, strProperty); _logger.debug("strDate1 = " + strDate1);

    boolean isStrDate0Empty = (strDate0 == null || strDate0.isEmpty());
    boolean isStrDate1Empty = (strDate1 == null || strDate1.isEmpty());

    if (isStrDate0Empty && isStrDate1Empty)
        return 0;
    // at least one of them is not empty    
    if (isStrDate0Empty)
        return -1;
    if (isStrDate1Empty)
        return 1;
    //none of them is empty
    return strDate0.compareTo(strDate1);
}
share|improve this answer
    
+1 good idea to record once the null/empty evaluation in a boolean, and use it to scan/validate all result possibilities. –  cl-r Jan 8 '13 at 13:45
up vote 2 down vote

Consider first testing if the strings point to the same object. You can drop the caseSensitive argument, this is directly from my Util class.

public static int compare(String s1, String s2, boolean caseSensitive) {
    if (s1 == s2) {
        return 0;
    } else if (s1 == null) {
        return -1;
    } else if (s2 == null) {
        return 1;
    } else {
        return caseSensitive?s1.compareTo(s2):s1.compareToIgnoreCase(s2);
    }
}
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.