Comparing two Strings which could be null or blank in a Comparator

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

    Is `Comparator.nullsFirst(String::compareTo).compare(date0, date1)` not sufficient?

  • burna

    burna Correct answer

    8 years ago

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

    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: http://stackoverflow.com/questions/481813/how-to-simplify-a-null-safe-compareto-implementation

    @burna: That is indeed much more readable. We aren't supposed to say "Thanks" in Stack Exchange, so I'll just say "Cheers".

    I suggest at least to avoid the isNotEmpty method. You could just switch the if and else case and continue using the isEmpty method.

License under CC-BY-SA with attribution


Content dated before 7/24/2021 11:53 AM

Tags used