Unit testing really simple implementations

As an independent subcontractor (freelance consultant) I get to work in various very different organisations, with very different approaches towards testing in general and unit-testing in particular.

Some places unit-testing is mandatory and there might even be a code-coverage tool where you must strive to meet a specific threshold.

At other places unit-tests are more like integration tests, where the unit-testing is considered implicit in the integration-unit-tests.

And then there are places where unit-tests are of your own choice, as long as you do not spend too much (measurable) time on them.

In the latter cases you as an experienced developer might feel that unit-testing very simple stuff is superfluous.

Not so! In my experience, it is when you do these simple no-brainer implementations that you make mistakes, simply because you do not have to think. Also, unit-testing simple utility stuff demonstrates corner-cases that might suddenly be used or mis-used in the code where changing even the simplest implementation might break important business logic

In this post I will go through a few very simple utility methods that I’ve worked on recently and talk about some of the finer details – and why unit-testing even the simplest is really important.

equals

I will present you with 2 very simple and quite similar equals utility methods, where 2 references are considered “pointing to the same object” if they are both null

Please be aware that this is obsolete since jdk 1.7 where the implementation is identical or similar to the java.util.Objects.equals() method.

The first is my favourite because of its simplicity:

/**
 * Returns true if a==b or a.equals(b) handling nulls. 
 * Note this is identical to the java 7 Objects.equals()
 *
 * @param <T>
 * @param a
 * @param b
 * @return
 * @see java.util.Objects#equals(Object, Object)
 */
public static <T> boolean equals(T a, T b) {
    return (a == b) || (a != null && a.equals(b));
}

The second is slightly more verbose but works just as well:

/**
 * Returns true if a==b or a.equals(b) handling nulls. 
 * Note this is similar to the java 7 Objects.equals()
 *
 * @param <T>
 * @param a
 * @param b
 * @return
 * @see java.util.Objects#equals(Object, Object)
 */
public static <T> boolean equals(T a, T b) {
    // both are null
    if (a == null && b == null) {
        return true;
    }
    // one is null but the other isn't
    if (a == null || b == null) {
        return false;
    }
    // none are null
    return a.equals(b);
}

They are both so simple they don’t need unit-testing, right?

Wrong. There is one very subtle difference between the two: If a and b both reference the same object which contains a defunct equals() implementation, the first version returns true (they are equal) and the second might return false.

Some might say it’s an advantage of the second that it (might) demonstrate if the implementation of T has a defunct equals() implementation. I don’t agree: our implementation should work under the assumption that others are playing by the rules. If not, there must be separate unit-tests to demonstrate they don’t.

A unit-test that highlights this issue is presented below. Assume the type A contains a simple integer that is used for equality testing in a naive equals() method. Assume that B extends from A and contains a defunct equals() method:

@Test
public void testEquals() {
    final A a1 = new A(1);
    final A a1alt = new A(1);
    final A a2 = new A(2);
    final B b = new B();
 
    assertFalse(Utils.equals(null, a1));
    assertFalse(Utils.equals(b, null));
    assertFalse(Utils.equals(a1, a2));
    assertFalse(Utils.equals(b, a2));
 
    assertTrue(Utils.equals(null, null));
    assertTrue(Utils.equals(a1, a1));
    assertTrue(Utils.equals(b, b)); // Here's the issue
    assertTrue(Utils.equals(a1, a1alt));
}

nvl

This operation is inspired by the Oracle database function with the same name. It basically returns the first non-null argument in a list of arguments. It is actually extremely useful.

First I present you with the simple but slightly problematic implementation. Again, it’s soo simple, why bother unit-testing it…

/**
 * Returns first not-null parameter, or null if all are nulls.
 * Inspired by Oracle database nvl function
 *
 * @param <T>
 * @param ts
 * @return
 */
public static <T> T nvl(T... ts) {
    for (int i = 0; i < ts.length; i++) {
        if (ts[i] != null) {
            return ts[i];
        }
    }
    return null;
}

What’s the issue with this? Well, if the method is called with no arguments or with a single null argument, it will throw a NPE:

@Test
public void testNvl() {
    assertNull(Utils.nvl()); // NPE
    assertNull(Utils.nvl(null)); // NPE
    assertNull(Utils.nvl(null, null));
    assertEquals(a1, Utils.nvl(a1));
    assertEquals(a1, Utils.nvl(null, a1, null));
    assertEquals(a1, Utils.nvl(null, a1, a2));
}

The proper implementation where we agree that no arguments or a single null argument should return null (we might also agree that the NPE above is fine, then there must be a unit-test for it…).

public static <T> T nvl(T... ts) {
    if (ts == null) {
        return null; // nino
    }
    for (int i = 0; i < ts.length; i++) {
        if (ts[i] != null) {
            return ts[i];
        }
    }
    return null;
}

The important part here is not really if the method handles no arguments or a null array. But when you are writing unit-tests, you are forced to think about the behavior of your operations, even the corner cases. One might argue that no one would ever call nvl() without arguments, but it might be used with a null array, in which case it actually works in a sensible way and the unit-test documents the behavior.

Please note that the nvl() above is not really an nvl method. The Oracle database function takes 2 arguments and that was what my initial implementation did too. And everywhere the nvl() method is used currently, it takes 2 arguments, one optional and a second constant, typically BigDecimal.ZERO…

The nvl() is actually a coalesce() operation, but I like the name of the nvl() method better…

A proper nvl() implementation would be similar to:

public static <T> T nvl(T a, T b) { return a == null ? b : a; }

public static <T> T nvl(T a, T b) { return a == null ? b : a; }

nullOrEmpty

So I was cleaning up some “old” code and everywhere I saw things like:

if (list == null || list.isEmpty()) {
// ...
}
if (map != null && !map.isEmpty()) {
// ...
}
if (s == null || "".equals(s)) {
// ...
}

…and got slightly annoyed. These constructs are simple and innocent, but just once you forget an “!” or use “||” instead of “&&” and you have a potential problem…

So I went for a single solution, the nullOrEmpty() method. And even if I didn’t have a need to support arrays I did it anyway, just in case I would ever need that. But I failed… Luckily I decided to unit-test the method, even if it is dead-simple. My first attempt is below – don’t copy it because it contains an error:

/**
 * Check for null or empty: Strings, Collections, Arrays...
 *
 * NOTE: This implementation contains an error
 * 
 * @param o
 * @return
 */
public static boolean nullOrEmpty(Object o) {
    if (o == null) {
        return true;
    }
    if (o instanceof String) {
        return ((String)o).isEmpty();
    }
    if (o instanceof Collection<?>) {
        return ((Collection<?>)o).isEmpty();
    }
    if (o instanceof Map<?,?>) {
        return ((Map<?,?>)o).isEmpty();
    }
    if (o.getClass().isArray()) {
        return Array.getLength(o) != 0;
    }
    return false;
}

So I wrote a unit-test, just to make sure I was sane… Which I wasn’t 🙂

@Test
public void testNullOrEmpty() {
   assertTrue(Utils.nullOrEmpty(null));
   assertTrue(Utils.nullOrEmpty(""));
   assertTrue(Utils.nullOrEmpty(Collections.emptyList()));
   assertTrue(Utils.nullOrEmpty(Collections.emptySet()));
   assertTrue(Utils.nullOrEmpty(Collections.emptyMap()));
 
   assertFalse(Utils.nullOrEmpty(1));
   assertFalse(Utils.nullOrEmpty(" "));
   assertFalse(Utils.nullOrEmpty(Collections.singletonList(a1)));
   assertFalse(Utils.nullOrEmpty(Collections.singleton(a1)));
   assertFalse(Utils.nullOrEmpty(Collections.singletonMap(b, a1)));
 
   assertTrue(Utils.nullOrEmpty(new Object[0])); // OOOPS!
   assertFalse(Utils.nullOrEmpty(new Object[1]));
}

The interesting part here is that the cases I wrote the method to handle (ie Strings, Collections and Maps) are all good. The Array case that I wrote “just in case” is not working, which makes this even more dangerous. Some other developer (or myself) might in the future take advantage of this feature and never ever think it is broken since it is used all over the place!

Again, the lesson learned is: UNIT-TEST! UNIT-TEST! UNIT-TEST! UNIT-TEST!!!!

The proper implementation is:

/**
 * Check for null or empty: Strings, Collections, Arrays...
 *
 * @param o
 * @return
 */
public static boolean nullOrEmpty(Object o) {
    if (o == null) {
        return true;
    }
    if (o instanceof String) {
        return ((String)o).isEmpty();
    }
    if (o instanceof Collection<?>) {
        return ((Collection<?>)o).isEmpty();
    }
    if (o instanceof Map<?,?>) {
        return ((Map<?,?>)o).isEmpty();
    }
    if (o.getClass().isArray()) {
        return Array.getLength(o) == 0; // <--- !!
    }
    return false;
}

So now you can write:

if (Utils.nullOrEmpty(list)) {
// ...
}
if (!Utils.nullOrEmpty(map)) {
// ...
}
if (Utils.nullOrEmpty(s)) {
// ...
}
if (!Utils.nullOrEmpty(array)) {
// ...
}

Nice and simple 😀

Note that the actual implementation is slightly different. The String testing is mostly done in the web-tier where a String containing only blanks is considered empty too, so the check is:

if (o instanceof String) { return ((String)o).trim().isEmpty(); }

if (o instanceof String) { return ((String)o).trim().isEmpty(); }

in

Okay, I really like the database in operator. Saying e.g. x in (1, 3, 5) is a lot easier to read than x = 1 OR x = 3 OR x = 5. And things gets worse in more complex expressions, e.g. "POST".equals(method) && (rc == 200 || rc == 204 || rc == 304).

So I decided I needed an in method.

In my existing project I need support for int‘s and enum‘s. I went for the following simple implementation:

public static boolean in(final T needle, T... haystack) {
    for (T t : haystack) {
        if (equals(needle, t)) {
            return true;
        }
    }
    return false;
}

Nice. Again while writing unit-tests, the first simple issues start to appear:

• What to do if needle is null?
• What if haystack is null?

As noted above, my current use-cases are for integers and enumerations and I do not have a need to be able to support nulls, not here nor there.

So the implementation became:

public static boolean in(final T needle, T... haystack) {
    if (needle == null || haystack == null) {
        return false;
    }
    for (T t : haystack) {
        if (equals(needle, t)) {
            return true;
        }
    }
    return false;
}

On the other hand, in the general case we might be interested in supporting null needles in the haystack. A slightly different implementation could be:

public static boolean in(final T needle, T... haystack) {
    if (haystack == null) {
        return needle == null;
    }
    for (T t : haystack) {
        if (equals(needle, t)) {
            return true;
        }
    }
    return false;
}

Both are reasonable but have different behavior and needs different unit-tests…

Conclusion

Writing unit-tests are really important. And there is no excuse, even the simplest stuff improves by being unit-tested. Often the simplest stuff (like the utilities mentioned above) is actually the easiest to unit-test. There is no excuse!

Unit-tests in general:

• Forces you to think about how the implementation behaves in the simplest and corner-cases, generally improving the quality.
• Improves quality because you ensure the implementation actually works in the cases it is written to handle.
• Documentation. Unit-tests documents how your operations are expected to work in different scenarios.
• Robustness. You might decide to support not-needed use-cases (as in the nullOrEmpty() handling arrays) and ensure that it actually does that.
• Stability. The next developer can do as (s)he pleases with your stuff, as long as the unit-tests are all green: “Refactorability”.

I’ve heard people claim that writing (proper) unit-tests takes about 50% of the total time to develop a feature. That might well be true, but the payback in terms of quality, documentation, robustness, stability and ease of “refactorabilty” is priceless!

Repeat after me: unit-test unit-test unit-test unit-test!

Look for the source here: https://github.com/judby/utils.blog