5
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ Is Comparator.nullsFirst(String::compareTo).compare(date0, date1) not sufficient? \$\endgroup\$ – Koekje Apr 24 '18 at 17:32
3
\$\begingroup\$

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);
}
\$\endgroup\$
  • 2
    \$\begingroup\$ 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/… \$\endgroup\$ – ZeroOne Jan 6 '13 at 0:45
  • \$\begingroup\$ @burna: That is indeed much more readable. We aren't supposed to say "Thanks" in Stack Exchange, so I'll just say "Cheers". \$\endgroup\$ – John Fitzpatrick Jan 6 '13 at 20:21
  • \$\begingroup\$ I suggest at least to avoid the isNotEmpty method. You could just switch the if and else case and continue using the isEmpty method. \$\endgroup\$ – tb- Jan 8 '13 at 11:32
6
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ +1 good idea to record once the null/empty evaluation in a boolean, and use it to scan/validate all result possibilities. \$\endgroup\$ – cl-r Jan 8 '13 at 13:45
4
\$\begingroup\$

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);
    }
}
\$\endgroup\$
1
\$\begingroup\$

A simpler strategy is to just canonicalize your strings that you plan to compare, then let compareTo() do it's thing. In this case, turn null strings into the empty string.

This code is untested, but conveys the idea.

private Integer compareDateStrings(BeanToDoTask arg0, BeanToDoTask arg1, String strProperty) {
    String strDate0 = getCanonicalProperty(arg0, strProperty); _logger.debug("strDate0 = " + strDate0);
    String strDate1 = getCanonicalProperty(arg1, strProperty); _logger.debug("strDate1 = " + strDate1);
    return strDate0.compareTo(strDate1); // neither strDateX can be null
}

private String getCanonicalProperty(BeanToDoTask arg, String strProperty) {
    String prop = BeanUtils.getProperty(arg, strProperty);
    return ( null == prop ) ? "" : prop;
}
\$\endgroup\$

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.