7
\$\begingroup\$

Sonar complains that I have too many return statements. How can I reduce the return statement count nicely?

public int compare(Sku o1, Sku o2) {
    if (o1 == null) {
        return (o2 == null) ? 0 : 1;
    } else if (o2 == null) {
        return -1;
    }

    if (o1.getSkuNumber() == null) {
        return o2.getSkuNumber() == null ? 0 : 1;
    } else if (o2.getSkuNumber() == null) {
        return -1;
    }

    return o1.getSkuNumber().compareTo(o2.getSkuNumber());
}
\$\endgroup\$
12
\$\begingroup\$

Since you state that you're using Java 8, you should use the variety of new Comparator static methods to extract the relevant fields for comparison:

public int compare(Sku o1, Sku o2) {
    Comparator<Sku> c1 = Comparator.nullsLast(null);
    return c1.thenComparing(Sku::getSkuNumber,
            Comparator.nullsLast(Comparator.naturalOrder())).compare(o1, o2);
}
  1. When Comparator.nullsLast(Comparator) is given a null argument, it considers two non-null argument to be equal. This is the basis of our first comparator.

  2. We then need to compare by getSkuNumber(), so we pass the method reference Sku::getSkuNumber to Comparator.thenComparing(Function, Comparator) as our second comparator. However, since SKU numbers may be null, we use a nested (or tertiary?) null-friendly comparator, Comparator.nullsLast(Comparator) again, as the second argument. The final comparator in use is just Comparator.naturalOrder(), which performs the final straightforward integer comparison if both SKU numbers are non-null.

edit: @Simon mentioned a good point that since your existing compare(Sku, Sku) method is likely to be the implementation for implementing Comparable<Sku>, you should consider making the Comparator I suggested above a public static final field, so that it doesn't have to be created every time, e.g.:

public static final Comparator<Sku> COMPARATOR = 
                        Comparator.nullsLast((Comparator<Sku>) null)
                            .thenComparing(Sku::getSkuNumber,
                                Comparator.nullsLast(Comparator.naturalOrder()));
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Note that the public int compare(Sku o1, Sku o2) method in itself is likely from a Comparator<Sku> implementation, so this Comparator thing that you create here should be made a public static final and used instead of the existing. Or it should at least be stored in a variable so that the comparator does not need to be created every time \$\endgroup\$ – Simon Forsberg Sep 10 '15 at 21:50
  • \$\begingroup\$ @SimonForsberg thanks for the suggested, have updated my answer. :) \$\endgroup\$ – h.j.k. Sep 11 '15 at 3:03
  • \$\begingroup\$ It is shorter, but not really readable \$\endgroup\$ – dieter Sep 11 '15 at 9:28
4
\$\begingroup\$

You are specifically asking about how to reduce the number of return statements, where one answer suggest a total rewrite, which most likely is a good option. However to tackle the general question of reducing the number of the return statements here are some ideas to play with:

  • Group similar returns using mulitple if conditions, or simplified tests
  • Store return result, before returning once at end
  • If possible, do a few early returns to avoid lots of testing and 'difficult' returns later on

Here is a modified variant where I simplified the tests using the simple fact that for the comparison you seem to consider o1 == null equal to o1.getSkuNumber() == null, and I test the getSkuNumber() only once, instead of multiple times. If getSkuNumber() was expensive, it also does make sense to avoid doing it multiple times.

public int compare(Sku o1, Sku o2) {
    // Simplify into two variables to use when testing 
    Comparable o1num = o1 != null ? o1.getSkuNumber() : null;
    Comparable o2num = o2 != null ? o2.getSkuNumber() : null;
    Integer result = null;

    // Simplified test, with storage of return result
    if ( o1num == null && o2num == null) {
      result = 0;

    } else if (o1num == null && o2num != null) {
      result = 1;

    } else if (o1num != null && o2num == null) {
      result = -1;
    }

    // If return result is already set, return it, else
    // do the actual comparison
    return (result != null) ? result : o1num.compareTo(o2num);
}

Whether it is worth it for such a simple case, is a matter of taste, but at least the trick to simplify the o1 vs o1.getSkuNumber() is a neat trick, and my first code example is a little gentler to my eye.

You could also opt for a not so readable, but shorter solution like:

public int compare(Sku o1, Sku o2) {
    // Simplify into two variables to use when testing 
    Comparable o1num = o1 != null ? o1.getSkuNumber() : null;
    Comparable o2num = o2 != null ? o2.getSkuNumber() : null;

    // Early return if either (or both) is null
    if ( o1num == null || o2num == null) {
      return o1num == null ? (o2num == null ? 0 : 1) :  -1 ;
    } 

    // Do the actual comparison
    return o1num.compareTo(o2num);
}

This last one simplifies the if structure by realising that all of the if's depend either one or both being null. Then if both are null return 0, or if only one return 1 or -1 depending which one is null. The second return statement, does the ordinary comparison as both are not null.

Edit: Changed into Comparable as it turns out that getSkuNumber() can return not only String. Further reduced the shortest solution, as suggested in comments, but kept an (unnecessary) level of parenthesis to keep some readability. :-)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ We don't know what's the type of getSkuNumber(), it can well be Integer, Long or a custom PositiveNumberWithDashes. ;) Also, the // Early return of your final suggestion can be slightly simplified further: return o2num == null ? o1num == null ? 0 : -1 : 1. When the if-condition is true, we just need to check if it's the second condition (o2num == null) that 'brings us' into the return statement. If that is the case, then we perform another 'check back' on the null-ness of o1num1 to determine which of the three values to return. ;) \$\endgroup\$ – h.j.k. Sep 14 '15 at 14:55
  • 1
    \$\begingroup\$ You are correct that it can be further reduced, but then it is really hard to read, I think. \$\endgroup\$ – holroy Sep 14 '15 at 15:00

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.