CodeSOD: It's Not That Different

We recently discussed the challenges of testing legacy code. Willy sent us some code from tests for a newer Java application.

Now, one of the main advantages, in my opinion, about unit-tests is that they serve as documentation. They tell you what the method does, and how it expects to be called. A good unit test is far better than a large pile of comments.

A bad unit test (or function supporting a unit test, as in this example) has the opposite effect.

public boolean checkDataByFilter(Map<String, String> filterValue, boolean equal) { boolean same = true; boolean different = false; try { Thread.sleep(3000); } catch (Exception e) {} List<Map<String, String>> mapList = getData(); for (Map<String, String> map : mapList) { for (String key : filterValue.keySet()) { if (!map.containsKey(key) || !map.get(key).equals(filterValue.get(key))) { if (equal) { same = false; } else { different = true; } break; } } if (!same || different) { break; } } return equal ? same : different; }

The core of this function is that it fetches data using getData and compares the results against filterValue, key by key. Then it returns true or false based on whether on not they match.

The first problem is that Thread.sleep(3000) call. That is very much a code smell in a unit test, it's doubly a code smell in a function that exists to support a unit test. If your unit test needs to sleep, well first off, it's not a unit test any more. But even if we admit the sleep, this clearly isn't where it belongs. The unit test method should worry about that kind of plumbing, not a supporting method which exists to perform a check. Speaking of, getData shouldn't happen here either.

But the real issue is the equal parameter and its implementation. Because this method can return true if the values are all the same, or if at least one value is different.

Now, first, I question the very idea of a method that negates its output based on a parameter you pass into it. That seems like a bad idea. But the logic required for this method is just a simple negation- return equal ? same : !same would accomplish the same goal.

The end result is that we have a method which makes our tests harder to understand, not easier. In the end, I wonder if the original developer just wanted to demonstrate their thorough understanding of what is the same, and different.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!