Learning Great Practices From Constraints

The Maryland high school I attended helped me get my early career off the ground. In 1980, I took my first computer class. A friend and his geek cohorts had already learned enough to swarm all over the HP/3000 machine we used. They quickly knew more about it than the operator hired by the school.

I learned that our machine and the terminals (HP-2640A’s, I believe) had been donated to the school. But before giving up the terminals, the donor cracked them open and removed some of the terminal memory, no doubt to help beef up the terminals for their newer system. The terminal memory allowed you to scroll back: by default, you got one page at a time (25×80, or maybe 24×80 to accommodate the f-keys line at the bottom). Later, when I started working at the University of Maryland–a job I got mostly because of my high school experience with the HP/3000–I had a wondrously souped-up terminal that allowed me to scroll back a whopping ten or so pages!

For whatever dumb reason, however, it was possible to remove even some of the first screen’s worth of memory. So instead of a 25×80 display, we effectively had a 17×80 display. You’d see 17 lines of output, followed out by a functionally-dead and black bottom third of the screen.

The effect this constraint had on editing programs was overwhelming. We used a line editor (TDP/3000 was the nicer one that HP offered), which meant you had to explicitly tell the editor to display certain lines when you wanted to view them. It went something like:
> list 34-49
… 16 lines of code displayed here …
>
The list command itself could scroll off the screen, but you still had a new command prompt taking up one more line, effectively leaving us with 16 lines maximum of code that I could see on-screen at any given time. I got very good at figuring out n:n+15.

The result: I learned very early on to compose my programs so that many, if not most, of my functions were short–no more than 16 lines! Otherwise, I’d spend far too much time listing lines appearing earlier or later in a function (to emulate scrolling up and down).

This habit became a key part of the successful parts of my programming career. My programs were usually structured to be very easy to maintain, and I never had lots of problems with pinpointing and fixing defects. Creating short methods is a great practice that I still find value in today.

Easier Testing Using the SRP

This simple file monitor class notifies listeners when a watched file is modified:

public class FileMonitor {
 private Set<FileChangeListener> listeners =
    new HashSet<FileChangeListener>();
 private int duration;
 private File file;
 private long lastModified;

 public FileMonitor(String filename, int duration) {
     this.file = new File(filename);
     lastModified = file.lastModified();
     this.duration = duration;
 }

 public void start() {
     boolean isDaemon = true;
     long initialDelay = duration;
     new Timer(isDaemon).schedule(
        new FileMonitorTask(), initialDelay, duration);
 }

 public void addListener(FileChangeListener listener) {
     listeners.add(listener);
 }

 class FileMonitorTask extends TimerTask {
     @Override
     public void run() {
         if (file.lastModified() > lastModified) {
             lastModified = file.lastModified();
             for (FileChangeListener listener: listeners) {
                 listener.modified();
             }
         }
     }
  }
}

A FileMonitor schedules a TimerTask that just checks the modified date from time to time, and compares it to the last modified date. The code above seems like a typical and reasonable implementation. FileMonitorTask could be an anonymous inner class, of course. I spent more time than I would have preferred test-driving this solution, since I made the mistake of test-driving it as a single unit. These tests had to deal with the vagaries of threading due to the Timer scheduling.

import java.io.*;
import java.util.*;
import org.junit.*;
import static org.junit.Assert.*;

public class FileMonitorTest {
 private static final String FILENAME = "FileMonitorTest.properties";
 private static final int MONITOR_DURATION = 1;
 private static final int TIMEOUT = 50;
 private FileMonitor monitor;
 private File file;
 private List<FileChangeListener> fileChangeNotifications;
 private Thread waitThread;

 @Before
 public void initializeListenerCounter() {
     fileChangeNotifications = new ArrayList<FileChangeListener>();
 }

 @Before
 public void createFileAndMonitor() throws IOException {
     FileUtils.writeFile(FILENAME, "property=originalValue");
     file = new File(FILENAME);
     monitor = new FileMonitor(FILENAME, MONITOR_DURATION);
 }

 @After
 public void deleteFile() {
     FileUtils.deleteIfExists(FILENAME);
 }

 @Test
 public void shouldNotifyWhenFileModified() throws Exception {
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(1);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();
     assertEquals(1, numberOfFileChangeNotifications());
 }

 @Test
 public void shouldSupportMultipleListeners() throws Exception {
     monitor.addListener(createCountingFileChangeListener());
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(2);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();
     assertEquals(2, numberOfFileChangeNotifications());
     assertAllListenersDistinctlyNotified();
 }

 @Test
 public void shouldOnlyReportOnceForSingleModification()
         throws InterruptedException {
     // slow--must wait on timeout. Better way?
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(2);

     monitor.start();

     alterFileToChangeLastModified();

     joinWaitThread();
     assertEquals(1, numberOfFileChangeNotifications());
 }

 @Test
 public void shouldReportMultipleTimesForMultipleModification()
     throws InterruptedException {
     monitor.addListener(createCountingFileChangeListener());
     waitForFileChangeNotifications(1);

     monitor.start();

     alterFileToChangeLastModified();
     joinWaitThread();

     waitForFileChangeNotifications(2);
     alterFileToChangeLastModified();
     joinWaitThread();

     assertEquals(2, numberOfFileChangeNotifications());
 }

 private FileChangeListener createCountingFileChangeListener() {
     return new FileChangeListener() {
         @Override
         public void modified() {
             fileChangeNotifications.add(this);
         }
     };
 }

 private int numberOfFileChangeNotifications() {
     return fileChangeNotifications.size();
 }

 private void waitForFileChangeNotifications(final int expected) {
     waitThread = new Thread(new Runnable() {
         @Override
         public void run() {
             while (numberOfFileChangeNotifications() != expected)             {
                 sleep(1);
             }
         }
     });
     waitThread.start();
 }

 private void sleep(int ms) {
     try {
         Thread.sleep(ms);
     } catch (InterruptedException exception) {
         fail(exception.getMessage());
     }
 }

 private void alterFileToChangeLastModified() {
     file.setLastModified(file.lastModified() + 1000);
 }

 private void joinWaitThread() throws InterruptedException {
     waitThread.join(TIMEOUT);
 }

 private void assertAllListenersDistinctlyNotified() {
     Set<FileChangeListener> notifications =
         new HashSet<FileChangeListener>();
     notifications.addAll(fileChangeNotifications);
     assertEquals(fileChangeNotifications.size(), 
                  notifications.size());
 }
}

Wow, that’s a bit longer than I would have hoped. Yes, there are better ways to test the threading. And there may be better threading mechanisms to implement the monitor, although I think the Timer is pretty simple. I don’t do enough threading, so I usually code a “familiar” solution (i.e. something I already know well) initially, and then refactor into a more expressive and concise solution. I’m pretty sure this test is flawed. But rather than invest in fixing it, I reconsidered my design (by virtue of wanting an easier way to test it).

The SUT is really two classes, the monitor and the task. Each one can easily be tested separately, in isolation, as a unit. Sure, I want to see the threading in action, but perhaps that’s better relegated to an integration test (which might also double as an acceptance test). I decided to change the solution to accommodate more focused and more robust tests. The file monitor tests are pretty much what was there before (with some small additional refactorings), except that there’s now no concern over threading–I just test the run method:

import java.io.*;
import org.junit.*;
import static org.mockito.Mockito.*;
import org.mockito.*;

public class FileMonitor_TaskTest {
 private static final String FILENAME = "FileMonitorTest.properties";
 private FileMonitor.Task task = new FileMonitor.Task(FILENAME);
 @Mock
 private FileChangeListener listener;

 @Before
 public void initializeMockito() {
     MockitoAnnotations.initMocks(this);
 }

 @BeforeClass
 public static void createFile() throws IOException {
     FileUtils.writeFile(FILENAME, "property=originalValue");
 }

 @AfterClass
 public static void deleteFile() {
     FileUtils.deleteIfExists(FILENAME);
 }

 @Before
 public void createTask() {
     task = new FileMonitor.Task(FILENAME);
 }

 @Test
 public void shouldNotNotifyWhenFileNotModified() {
     task.addListener(listener);
     task.run();
     verify(listener, never()).modified();
 }

 @Test
 public void shouldNotifyWhenFileModified() throws Exception {
     task.addListener(listener);
     changeFileLastModifiedTime();

     task.run();

     verify(listener, times(1)).modified();
 }

 @Test
 public void shouldSupportMultipleListeners() throws Exception {
     task.addListener(listener);
     FileChangeListener secondListener = mock(FileChangeListener.class);
     task.addListener(secondListener);

     changeFileLastModifiedTime();
     task.run();

     verify(listener, times(1)).modified();
     verify(secondListener, times(1)).modified();
 }

 @Test
 public void shouldOnlyReportOnceForSingleModification() 
         throws InterruptedException {
     task.addListener(listener);

     task.run();
     changeFileLastModifiedTime();
     task.run();
     task.run();

     verify(listener, times(1)).modified();
 }

 @Test
 public void shouldReportMultipleTimesForMultipleModification() 
          throws InterruptedException {
     task.addListener(listener);

     task.run();
     changeFileLastModifiedTime();
     task.run();
     changeFileLastModifiedTime();
     task.run();

     verify(listener, times(2)).modified();
 }

 private void changeFileLastModifiedTime() {
     File file = new File(FILENAME);
     file.setLastModified(file.lastModified() + 1000);
 }
}

I introduced Mockito to provide a simple verifying stub for the listener. I suppose I should also stub out interactions with File, but I’ll incur the speed penalty for now. Next, I needed to test the remaining FileMonitor code. To do this involved proving that it creates and schedules a task appropriately using a timer, and that it handles listeners appropriately.

import xxx.CollectionUtil;
import java.util.Timer;
import org.junit.*;
import org.mockito.*;
import static org.mockito.Mockito.*;

public class FileMonitorTest {
 @Mock
 private Timer timer;

 @Before
 public void initializeMockito() {
     MockitoAnnotations.initMocks(this);
 }

 @Test
 public void shouldScheduleTimerWhenStarted() {
     String filename = "filename";
     long duration = 10;
     FileMonitor monitor = new FileMonitor(filename, duration) {
         @Override
         protected Timer createTimer() {
             return timer;
         }
     };
     monitor.start();
     verify(timer).schedule(any(FileMonitor.Task.class), 
         eq(duration), eq(duration));
 }

 @Test
 public void shouldDelegateListenersToTask() {
     FileChangeListener listener = mock(FileChangeListener.class);
     FileMonitor monitor = new FileMonitor("", 0);
     monitor.addListener(listener);

     CollectionUtil.assertContains(
         monitor.getTask().getListeners(), listener);
  }
}

The design of the production code had to change based on my interest in better tests. A number of small, incremental refactoring steps led me to this solution:

import java.io.*;
import java.util.*;

public class FileMonitor {
 private final long duration;
 private Task task;

 public FileMonitor(String filename, long durationInMs) {
     task = new Task(filename);
     this.duration = durationInMs;
 }

 public void start() {
     long initialDelay = duration;
     createTimer().schedule(task, initialDelay, duration);
 }

 protected Timer createTimer() {
     final boolean isDaemon = true;
     return new Timer(isDaemon);
 }

 Task getTask() {
     return task;
 }

 public void addListener(FileChangeListener listener) {
     task.addListener(listener);
 }

 static class Task extends TimerTask {
     private final File file;
     private long lastModified;
     private Set<FileChangeListener> listeners = 
         new HashSet<FileChangeListener>();

     Task(String filename) {
         this.file = new File(filename);
         lastModified = file.lastModified();
     }

     public void addListener(FileChangeListener listener) {
         listeners.add(listener);
     }

     Set&lt;FileChangeListener&gt; getListeners() {
         return listeners;
     }

     @Override
     public void run() {
         if (file.lastModified() > lastModified) {
             lastModified = file.lastModified();
             for (FileChangeListener listener: listeners) {
                 listener.modified();
             }
         }
     }
 }
}

Well, most interestingly, I no longer have any tests that must deal with additional threads. I test that the monitor delegates appropriately, and I test that the task works properly when run. I still want the integration tests, but I think they can be relegated to a higher, acceptance level test for the story:

reload properties at runtime

Otherwise, at this point, I have a high level of confidence in my code. I know the Timer class works, and I know my two classes work, and I’ve also demonstrated to myself that they all work together. I’m ready to move on. Some additional relevant points:

  • Threading is tough to get right, and it can be even tougher to test it.
  • My final solution is a few more lines of code due to supporting my interest in more focused testing. I’m ok with the additional code because of the flexibility having the tests gives me, and more often than not, having them allows me to easily eliminate duplication elsewhere.
  • It’s hard to be overdo the SRP. Far more classes than not weigh in the wrong direction of too many responsibilities.
  • The original solution, probably fine otherwise, is what you might expect from someone uninterested in TDD. It works (I think) but it represents rigid code, code that is more costly to change.

 

Cute Little Abstractions

I’m refactoring several classes of over 2000 lines each. These are classic god-classes that mediate a handful of stubby little data-centric classes around them. I reduced the first, primary domain class by over 1100 lines, to under 1500, by simply pushing responsibilities around. Adding tests after the fact (using WELC techniques, of course) gave me the confidence to refactor. In just a few hours, I’ve been able to completely eliminate about 700 lines so far out of the 1100 that were shifted elsewhere. I’m loving it!

After cleaning up some really ugly code, I ended up with better (not great) code:

private boolean containsThreeOptionsSameType() {
   Map<String, Integer> types = new HashMap<String, Integer>();
   for (Option option : options)
      increment(types, option.getType());
   return count(types, Option.ALPHA) >= 3 || 
          count(types, Option.BETA) >= 3 || 
          count(types, Option.GAMMA) >= 3 || 
          count(types, Option.DELTA) >= 3;
}

private int count(Map<String, Integer> map, String key) {
   if (!map.containsKey(key))
      return 0;
   return map.get(key);
}

private void increment(Map<String, Integer> map, String key) {
   if (!map.containsKey(key))
      map.put(key, 1);
   else
      map.put(key, map.get(key) + 1);
}

I could clean up the complex conditional on the return statement (perhaps calling a method to loop through all types). Also, the count and increment methods contain a bit of duplication.

More importantly, the count and increment methods represent a missed abstraction. This is very common: We tend to leave little, impertinent, SRP-snubbing, OCP-violating methods lying about, cluttering up our classes.

(Aside: I’m often challenged during reviews about exposing things so I can test them. Long-term, entrenched developers are usually very good about blindly following the simple rule about making things as private as possible. But they’re perfectly willing to throw OO concept #1, cohesion, completely out the window.)

I decided to test-drive the soon-to-be-no-longer-missing abstraction, CountingSet. I ended up with an improved implementation of increment by eliminating the redundancy I spotted above:

import java.util.*;

public class CountingSet<T> {
   private Map<T, Integer> map = new HashMap<T, Integer>();

   public int count(T key) {
      if (!map.containsKey(key))
         return 0;
      return map.get(key);
   }

   public void add(T key) {
      map.put(key, count(key) + 1);
   }
}

Some developers would be appalled: “You built a cruddy little class with just four lines of real code? And for only one client?” And further: “It’s only a subset of a counting set, where are all the other methods?”

Sure thing, buddy. Here’s my client:

private boolean containsThreeOptionsSameType() {
   CountingSet<String> types = new CountingSet<String>();
   for (Option option : options)
      types.add(option.getType());
   return types.count(Option.ALPHA) >= 3 || 
          types.count(Option.BETA) >= 3 || 
          types.count(Option.GAMMA) >= 3 || 
          types.count(Option.DELTA) >= 3;
}

My client now only contains intent. The small ugliness with the parameterized map and boxing is now “information hidden.” I also have a simple reusable construct–I know I’ve had a similar need several times before. As for building and/or using a fully-functional CountingSet, there’s something to be said for creating “adapter” classes with the smallest possible, and most meaningful, interface.

Good enough for now. Don’t hesitate to create your own cute little abstractions!

I Know What You Did

It takes about two seconds to determine whether or not someone used TDD to derive a set of unit tests. While it’s possible for someone to write as high-quality unit tests using TAD (test-after development), it just doesn’t happen. Invariably I see many of the same problems, most significant of which is that the TAD tests don’t cover nearly enough of the cases.

Most of the time, developers doing TAD simply refuse to follow the single-responsibility principle (SRP) at the class or method level. (Despite bizarre claims by Jeff Atwood and Joel Spolsky, asking people to push in the direction of Uncle Bob’s five principles of class design is a reasonable and sensible thing to do.) They build large classes and large methods–not small, cohesive, potentially reusable units–and then complain about the difficulty of testing them.

My simple answer for now is, “How can you expect to write unit tests when you don’t have any units?”

Atom