Pragmatic Unit Testing in Java 8 with JUnit (2015)

Part 3. The Bigger Design Picture

Chapter 9. Bigger Design Issues

In the last chapter we refactored the matches() method into a number of clearer, more-composed methods. Such continual refactoring of small bits of code helps to keep code maintenance costs low.

Writing unit tests isn’t an exercise that occurs in a vacuum. It’s instead part of the larger, continually shifting puzzle we call design. Our system’s design impacts our ability to write tests, and vice versa.

In this chapter we’ll take a look at some bigger design concerns. Specifically, we’ll focus on the Single Responsibility Principle (SRP), which guides us to small classes that increase flexibility and ease of testing, among other things. And we’ll investigate command-query separation, which uses methods that don’t end up fooling their users by both creating side effects and returning values. We’ll apply these principles by refactoring code in the Profile class.

The Profile Class and the SRP

Let’s take a look at our Profile class so far:

At under a hundred source lines, Profile isn’t inordinately large and doesn’t seem excessively complex. But it contains some hints that the class exhibits less-than-ideal design.

Profile tracks and manages information for a company or person, including a name and a collection of answers to questions. This set of information that the Profile class captures will most likely need to change over time—more information will likely need to be added, and some might need to be removed or altered.

A secondary responsibility of the Profile class is to calculate a score to indicate if—and to what extent—a set of criteria matches the profile. With the refactoring we accomplished in the previous chapter, we ended up with a good number (five) of methods to assist in scoring matches. Changes to the Profile class are thus likely for a second reason: we’ll undoubtedly change the sophistication of our matching algorithm over time.

The Profile class violates the Single Responsibility Principle (SRP) of object-oriented class design, which tells us that classes should have only one reason to change. (The SRP is one of five important class-design principles—see SOLID Class-Design Principles.) The resulting focus of a class on a single responsibility decreases the risk of change: the more responsibilities a class has, the easier it is to break other existing behavior when changing code within the class. Smaller, more-focused classes are also more likely to provide value in another context—reuse! In contrast, a very large class with lots of responsibilities cannot possibly be used in other contexts.

SOLID Class-Design Principles

In the mid-1990s, Robert C. Martin gathered five principles for object-oriented class design, presenting them as the best guidelines for building a maintainable object-oriented system. Michael Feathers attached the acronym SOLID to these principles in the early 2000s.

· Single Responsibility Principle (SRP). Classes should have one reason to change. Keep your classes small and single-purposed.

· Open-Closed Principle (OCP). Design classes to be open for extension but closed for modification. Minimize the need to make changes to existing classes.

· Liskov Substitution Principle (LSP). Subtypes should be substitutable for their base types. From a client’s perspective, override methods shouldn’t break functionality.

· Interface Segregation Principle (ISP). Clients should not be forced to depend on methods they don’t use. Split a larger interface into a number of smaller interfaces.

· Dependency Inversion Principle (DIP). High-level modules should not depend on low-level modules; both should depend on abstractions. Abstractions should not depend on details; details should depend on abstractions.

You can and should read more about SOLID at Wikipedia.^[32]

Extracting a New Class

The Profile class defines two responsibilities:

· Track information about a profile.

· Determine whether and to what extent a set of criteria matches a profile.

We want to split the two responsibilities so that we have two classes, each small and adherent to the SRP. To do so, we plan to extract the code related to the matches responsibility to another class, named MatchSet. As with all refactoring, we seek an incremental path—make a small change, run the tests to make sure they still pass.

The first change: move the calculateScore() logic into MatchSet. Start by changing the code in matches() to declare the intent. Rather than call calculateScore() directly from matches(), construct a new MatchSet object with the information it needs—the hash map of answers and the criteria—and ask it for the score:

Copy the calculateScore() method into MatchSet and then whittle the class a bit: in the constructor of MatchSet, store the answers argument in a field, and pass the criteria instance to the calculateScore() method. Add a score field and a getScore() method to return it.

Compilation reveals that calculateScore() needs to call answerMatching(). Copy over that method:

Both classes now compile. The code in Profile no longer uses the calculateScore() private method. Delete it. The answerMatching() method is still used by code in Profile; make a note that it’s duplicate code. If the answerMatching() method still needs to be used by both classes when you finish moving code about, you’ll have to figure out how to factor that code to a single place.

The score-related code is now in MatchSet. The remainder of the code in matches() represents the second goal of the method—to answer true or false depending on whether or not the criteria match the set of answers. We decide to delegate the responsibility for coming up with the answer to the MatchSet class.

First step: create the matches() method in MatchSet. Move into it the two lines from matches() in the Profile class. The two methods it calls, doesNotMeetAnyMustMatchCriterion() and anyMatches(), must come along for the ride. Here’s matches() in its new home:

For the Profile method matches() to delegate to the MatchSet implementation of matches(), create a matchSet local variable and call the matches() method on it after storing the score:

Back in MatchSet, the moved doesNotMeetAnyMustMatchCriterion() and anyMatches() methods both require access to the criteria instance. Alter the constructor in MatchSet to store criteria as a new field. Here’s MatchSet with everything moved successfully:

The MatchSet class has all the code it needs to handle processing of match requests. Because criteria is now stored in a field, there’s no reason to pass criteria around to the calculateScore(), doesNotMeetAnyMustMatchCriterion(), and anyMatches() methods:

The concept of real-world modeling in object-oriented design gets you only so far. If you constrain yourself to a single Profile class because it matches well to the real-world concept of profiles, you do yourself a disservice. Your classes become larger and more complex. That in turn minimizes reuse, increases the difficulty of understanding what each class does, and increases the likelihood of breaking unrelated items each time a class is edited.

Create classes that map to concepts, not concrete notions. The MatchSet concept allows you to isolate the code related to matching, which keeps its code simpler. The Profile code from which it came gets simpler as well.

Design is everywhere you make a code change. Focus on all aspects of maintenance, not just class-level interactions. Let’s take a look at the design space for an individual method and discuss the concept of command-query separation.

Command-Query Separation

In Profile, we scrutinize the matches() method:

It has the awkward side effect of storing a calculated score on the Profile object. That makes no sense from the context of a Profile. A Profile doesn’t have a single score; it only has a score in conjunction with an attempt to match on criteria.

The score side effect causes another problem, which is that we can’t separate one interest from the other. If we want the score, we have to know to call the matches() method, which is counterintuitive, and we wastefully discard the Boolean result. Conversely, to know if a set of criteria matches, we end up unwittingly altering a Profile attribute (score).

A method that both returns a value and generates a side effect (changes the state of the class or some other entity in the system) violates the principle known as command-query separation. The principle states that a method should either execute a command (do something that creates a side effect) or answer a query (return some value), but not both.

In some cases, command-query separation creates potential pain for client code. If a query method alters the state of the object, it might not be possible to call it twice (to ask the same question again, for whatever good reason) and get the same answer. Or, calling it a second time might alter the state of the object in an undesired way.

A classic example of the violation of command-query separation exists in the java.util.Iterator interface. The next() method returns the object pointed to and advances the current object pointer. Careless use can lead to “duh!” defects.

We decide that it’s the job of the client code to deal with MatchSet objects however they want. As a result, we change the interface to Profile to contain a method that simply returns a new MatchSet object when passed a Criteria instance. The client can itself get the score or the Boolean answer (as to whether or not the criteria matches) from the MatchSet.

Accordingly, delete the score() method and the score field from Profile. The resulting class is a good example of adherence to the SRP:

Uh oh. That change created a few problems in the tests, and several are now failing. That’s not good. We must fix them before going any further.

The Cost of Maintaining Unit Tests

The change to the interface to Profile broke a number of tests in ProfileTest. We need to invest some effort to fix the tests, which points out one of the costs of having unit tests in the first place.

Refactoring is supposed to be an activity where we change the implementation of the code without changing its behavior. The tests are supposed to be a reflection of the behavior. But the reality is that we are changing the behavior of our classes, at least in terms of how we expose that behavior through the classes’ interfaces.

We accept the cost of fixing broken tests because their return on value can be far greater. We’ve mentioned elsewhere the benefits of having code with few defects, the benefit of being able to make changes without the worry about having broken other code, and the benefit of knowing exactly what the code does (without having to spend inordinate amounts of time digging through the code and possibly guessing wrong).

Still, the cost of maintaining the tests isn’t tiny. We truly recognize its expense when we encounter scenarios like the current one, where we’ve broken a number of tests all at once.

Moving forward, think about the magnitude of failing tests as a negative design indicator: the more tests that break simultaneously, the more likely you have a design issue.

How to Protect Yourself

Duplication of code is one of the biggest design problems. From the stance of the tests themselves, duplication across tests creates two problems: first, it makes the tests harder to follow. If you expend three lines of code to create and populate an Answer object, it’s three lines that a reader must step through and understand. Distilling them to a single concept, such as a helper method named createMatchingAnswer(), imparts immediate understanding to the reader.

Second, extracting duplicated occurrences of small bits of code to a single method minimizes the impact when those small bits must change. Better to make a change in a single place than in numerous tests scattered across your source base.

Requiring several or even dozens of lines of code to set up unit tests is an indicator that you have problems in the design of your system. Violation of the SRP means larger classes, which usually lead to more dependencies on other classes, which in turn demands more effort to set up your tests. Find a way to split your larger classes!

The compulsion to test private methods—implementation details—is another hint that your classes are too large. More often than not, a spate of private methods suggests that the private behavior is better moved to a new class where it becomes public behavior.

If unit testing seems hard, take the hint. Find ways to make unit testing easier by improving your design. You’ll decrease (but never eliminate) the cost of maintaining your tests.

Fixing Our Broken Tests

The current tests in ProfileTest are mostly focused on managing what are now MatchSet objects. Extract these tests to the new MatchSetTest test class and make the changes necessary to get the test code compiled and passing. Specifically, to create a MatchSet object, we must pass it a hash of question-text-to-Answer-object. Add a utility method to simplify creating MatchSet objects and another to simplify adding Answer objects to a MatchSet.

Here’s what a couple of the tests look like in their new home:

When you extract code to new classes, the tests you write become more direct and often simpler to write. To test MatchSet code, the tests no longer require the distracting overhead of creating Profile objects. You also tend to cover more permutations when the tests are easier to write.

If you move private methods to become public methods on a new class, you’ll find that they typically have insufficient test coverage—because it’s harder to test private behavior. After the methods become public, it’s your job to ensure that you document the newly exposed behavior by writing tests against it.

Other Design Thoughts

The MatchSet() constructor does the work of calculating the score. If the calculated score isn’t consumed by a client, the effort to compute it is waste. For this reason (among others^[33]), avoid doing any real work in constructors.

Change the code to calculate the score when it’s requested:

The score field goes away, and the calculateScore() method gets inlined into getScore(). If recalculating the score each time getScore() gets called is a performance sink, you can always introduce lazy initialization to fix the problem.

The way we handle the answers collection raises a few questions. In Profile, we create a Map<String, Answer> that stores answers using the question text as a key. But we also pass the answers map reference into each MatchSet object created. That means both classes have intimate knowledge of how answers get stored and retrieved. Implementation details scattered across classes foster the code smell known as Shotgun Surgery:^[34] if/when we need to replace the answers map with a database table, we’ll end up having to make that change in a couple of places.

Having the answers map in two places also introduces confusion about the state of the data. Is it somehow possible for the Profile to contain a different set of answers from a MatchSet? (With the current code, no, but as code changes, this is how to end up with defects.)

We decide to isolate the storage of answers to a class named AnswerCollection. We refactor incrementally, running tests with each small change, and end up with the following code:

Finally: MatchSet still contains redundant loops to iterate over the criterion objects in a criteria collection. Although our implementation works, it does carry a performance penalty, and it also represents the duplication of multiple methods needing to specify the iteration. You might consider introducing the Visitor design pattern,^[35] which solves the problem without reverting the code to the original mess of an entangled loop that does everything.

Keep a critical eye on your system’s design, and remember that there’s rarely one “best” possible design. Your responsibility to keep your system clean never ends.

After

You’ve already heard this, but it can’t be stressed enough:

In this chapter we focused on improving our design based on a couple of big design ideas: the SRP and command-query separation. You owe it to yourself to know as much as possible about these and other big concepts in design. You also owe it to yourself to understand the “little” concepts in design and how small code refactorings can make a big difference. Armed with a stockpile of design smarts, your unit tests will allow you to refactor your code to a place where it more readily supports the inevitable changes coming.

Be willing to create new, smaller classes and new, smaller methods. It’s a bit of effort (though tools like Eclipse make it much easier), and we often resist as lazy programmers. But it’s worth it: design flexibility starts with smaller, more-composed building blocks.

We’d like to test more of our code, but the realities of what our code must interact with (things like databases and services) mean that it won’t always be easy to write unit tests. We’ll next talk about how to overcome these real challenges by using mock objects.

Footnotes