Refactoring and Performance

In the first portion of the book Refactoring, Martin Fowler presents an example where he whittles a verbose method into an improved design. Here’s the original, more or less:

   public String statement() {
      double totalAmount = 0;
      int frequentRenterPoints = 0;
      Enumeration rentals = this.rentals.elements();
      String result = "Rental Record for " + getName() + "\n";

      while (rentals.hasMoreElements()) {
         double thisAmount = 0;
         Rental each = (Rental)rentals.nextElement();
         switch (each.getMovie().getPriceCode()) {
            case Movie.REGULAR:
               thisAmount += 2;
               if (each.getDaysRented() > 2)
                  thisAmount += (each.getDaysRented() - 2) * 1.5;
               break;
            case Movie.NEW_RELEASE:
               thisAmount += each.getDaysRented() * 3;
               break;
            case Movie.CHILDRENS:
               thisAmount += 1.5;
               if (each.getDaysRented() > 3)
                  thisAmount += (each.getDaysRented() - 3) * 1.5;
               break;
         }

         frequentRenterPoints++;

         if (each.getMovie().getPriceCode() == Movie.NEW_RELEASE &&
             each.getDaysRented() > 1)
            frequentRenterPoints++;

         result += "\t" + each.getMovie().getTitle() +
                   "\t" + String.valueOf(thisAmount) + "\n";
         totalAmount += thisAmount;
      }
   }

And here’s the (relevant portion of the) factored code. I think this is as far as Fowler takes the example:

   public void addRental(Rental rental) {
      rentals.add(rental);
   }

   public String statement() {
      StringBuilder result = new StringBuilder();
      appendHeader(result);
      for (Rental rental: rentals)
         appendDetail(result, rental);
      appendSummary(result);
      return result.toString();
   }

   private int calculateFrequentRenterPoints() {
      int totalFrequentRenterPoints = 0;
      for (Rental rental: rentals)
         totalFrequentRenterPoints += rental.getFrequentRenterPoints();
      return totalFrequentRenterPoints;
   }

   private double calculateTotalCost() {
      double totalCost = 0;
      for (Rental rental: rentals)
         totalCost += rental.getCost();
      return totalCost;
   }

The while loop in the original statement method intertwines three things: construction of a text statement, addition into a total cost, and addition into a total for frequent renter points. The refactored version separates and isolates each of these three behaviors.

At the point of presenting this code, Fowler discusses the performance implications: Instead of a single set of iterations, the code now requires three iterations over the same collection. Many developers object strongly to this refactoring.

I’ve done quick (and rough, but relatively accurate) performance measurements for this example. Things aren’t three times worse, as a naive guess might suggest; instead we’re talking about a 7% degradation in performance. That’s still a significant amount in many systems, maybe even too much to accept. In many other systems, the degradation is negligible, given the context.

Some conclusions: First, always measure, never guess. Second, understand what the real performance requirements are. Create end-to-end performance tests early on. Run these regularly (nightly?), so that you know the day someone introduces underperforming code.

The refactored, better design provides the stepping stone to many other good things. The small, cohesive methods can each be read and understood in a few seconds. You can glance at each, and almost instantly know they must be correct. They’re also extremely easily tested if you aren’t so trustworthy of glances.

Looking further at the code, you might also spot an abstraction waiting to come into existence–the collection of rentals itself. Producing a statement is a responsibility separate from managing this collection. You could create another class named RentalSet or RentalCollection. Moving the calculation operations into the new class is a trivial operation, given that the calculation logic is now completely divorced from statement generation.

Other benefits accrue. Fowler talks about the ability to provide support for an HTML statement, in addition to existing support for a plaintext statement. Migration to a polymorphic hierarchy is simple and safe with the refactored design.

Following the mantra of “make it run, make it [the design] right, make it fast,” you can return to the performance issue now that you have an improved design. Suppose you must restore performance and eliminate the 7% degradation. You could un-factor the loop extract and again intertwine three elements in the body of one for loop.

But since you have a simpler design, the optimization doesn’t need to impact the design so negatively:

   public void addRental(Rental rental) {
      rentals.add(rental);

      // optimization--cache totals:
      totalFrequentRenterPoints += rental.getFrequentRenterPoints();
      totalCost += rental.getCost();
   }

   private int calculateFrequentRenterPoints() {
      return totalFrequentRenterPoints;
   }

   private double calculateTotalCost() {
      return totalCost;
   }

(Note the rare, debatably worthwhile comment, which I could eliminate by another method extract.)

Small methods are about many things, ranging from comprehension to reuse. In this case, having an optimized design allowed for an even simpler solution to the optimization of performance.

Posted in agile, coding, performance | 1 Comment

Test-Driving a Heap-Based Priority Queue

It’s been over 25 years since I had to code a priority queue. Should I really need to? Java now supplies a priority queue, and an important lesson of software development is that you should first seek to reuse what you can. But it’s worthwhile to know how to build such things yourself, in case you’re in an environment where one doesn’t exist.

As usual, though, I’m mostly interested in honing my ability to incrementally grow things, including interesting data structures. What can I learn from test-driving a priority queue?

All a priority queue does is return elements from a queue in priority order. Per the Wikipedia page, there are many ways to implement a priority queue, including a couple bad ways. Right off the bat, there’s no way I could force a priority queue to use a binary heap–one of the better choices–given only the client needs. Test-driving a solution and building the “simplest” possible implementation would result in the worst-performing solution.

So how to force the issue? Well, the nice thing about a binary heap is that it has a very concisely defined structure, one that I can verify easily. (I’ll refer to a binary heap as simply a heap for the rest of this article.) A binary heap is a tree where:

  • The key for each node is greater than or equal to each of its children.
  • The tree is complete–the last level is filled left-to-right, and all other levels are filled.

I know I want to end up with a heap–I need the performance–and will use that as a guiding factor in test-driving a solution. Does this violate YAGNI? Maybe. Other options? I could introduce a performance-related test, one where I tried to verify the big-O of each of the three operations (adding a new item, peeking at the highest priority item, and removing the highest priority item–the last two of which are often combined into a poll method). That would be tough to do well, and still might not guarantee an implementation that used a binary heap.

No, I need to be able to peek at the implementation. TDD will still be a great help. I still want to build the heap incrementally–there’s a decent amount of code involved, and plenty of opportunities to get it wrong. I’ll start simple, however, and not worry about heap characteristics until I get basic queue functionality working:

import org.junit.*;
import static org.junit.Assert.*;
import static org.hamcrest.CoreMatchers.*;

public class PriorityQueueTest {
   private static final Object ITEM_50 = "50";
   private static final Object ITEM_100 = "100";

   PriorityQueue queue;

   @Before
   public void create() {
      queue = new PriorityQueue();
   }

   @Test
   public void emptyWhenCreated() {
      assertThat(queue.isEmpty(), is(true));
   }

   @Test
   public void noLongerEmptyAfterAdd() {
      queue.add(ITEM_50, 50);

      assertThat(queue.isEmpty(), is(false));
   }

   @Test
   public void singletonQueueReturnsSoleItemOnPoll() {
      queue.add(ITEM_50, 50);

      assertThat(queue.poll(), is(ITEM_50));
   }

   @Test
   public void isEmptyAfterSoleElementRemoved() {
      queue.add(ITEM_50, 50);
      queue.poll();

      assertThat(queue.isEmpty(), is(true));
   }

   @Test
   public void returnsFIFOWithTwoAlreadyOrderedItems() {
      queue.add(ITEM_100, 100);
      queue.add(ITEM_50, 50);

      assertThat(queue.poll(), is(ITEM_100));
      assertThat(queue.poll(), is(ITEM_50));
      assertThat(queue.isEmpty(), is(true));
   }
}

I won’t bore you with the rote, incremental steps used to get to this point. For now the solution is simply a class that wraps an ordered list. There’s no support for pulling things out in other than FIFO order, since there are yet no tests that demand the capability.

import java.util.*;

public class PriorityQueue {
   private List<Object> heap = new ArrayList<Object>();

   public boolean isEmpty() {
      return heap.isEmpty();
   }

   public void add(Object item, int priority) {
      heap.add(item);
   }

   public Object poll() {
      return heap.remove(0);
   }
}

All those tests to get to this trivial bit of code? No big deal. About five minutes of work, and I’ve gotten some good cases documented and clamped down.

The next test makes things a little more interesting:

   @Test
   public void returnsInPriorityOrder() {
      queue.add(ITEM_50, 50);
      queue.add(ITEM_100, 100);

      assertThat(queue.poll(), is(ITEM_100));
      assertThat(queue.poll(), is(ITEM_50));
      assertThat(queue.isEmpty(), is(true));
   }

The implementation now requires actually using the priority, associating it with each item, hence the introduction of the Node class. It also means I need a little bit of “real heap logic” in the add method: If the second item is higher priority than the first, sift it to the front. Since there are only two items, it’s a hard-coded swap. The way I coded it also happens to work for single-element heaps.

import java.util.*;

public class PriorityQueue {
   private List<Node> heap = new ArrayList<Node>();

   class Node {
   final Object object;
   final int priority;

   Node(Object object, int priority) {
      this.object = object;
      this.priority = priority;
   }
   }

   public boolean isEmpty() {
      return heap.isEmpty();
   }

   public void add(Object item, int priority) {
      Node node = new Node(item, priority);
      heap.add(node);
      if (node.priority > heap.get(0).priority)
         siftUp();
   }

   private void siftUp() {
      Node temp = heap.get(0);
      heap.set(0, heap.get(1));
      heap.set(1, temp);
   }

   public Object poll() {
      return heap.remove(0).object;
   }
}

Interestingly, I still don’t have a proper tree with parents and children–just a list with a couple slots filled in. In looking about at ways to implement a binary tree, the most appealing is to simply build it in-place in an array–something easily done given the constraint that the tree must be complete. Using an array will make it easy (and fast) to navigate from parent to child and vice versa.

The tests could use a bit of clarity. I add a helper method to simplify my assertion (and distill it to one line):

   @Test
   public void returnsFIFOWithTwoAlreadyOrderedItems() {
      queue.add(ITEM_100, 100);
      queue.add(ITEM_50, 50);

      assertQueuePollingOrder(ITEM_100, ITEM_50);
   }

   @Test
   public void returnsInPriorityOrder() {
      queue.add(ITEM_50, 50);
      queue.add(ITEM_100, 100);

      assertQueuePollingOrder(ITEM_100, ITEM_50);
   }

   private void assertQueuePollingOrder(Object... expectedItems) {
      List<Object> items = new ArrayList<Object>();
      while (!queue.isEmpty())
         items.add(queue.poll());
      assertThat(items, is(Arrays.asList(expectedItems)));
   }

Time to move on to the more interesting cases.

Thinking a bit about the next test (thinking is allowed, in fact encouraged, in TDD, particularly when determining what to do next)… Adding a third element to the heap will require a bit more generalized logic, and a fourth element (which would represent the third level of the heap’s tree) even more so. All complex enough without worrying about the complexity of removal, which requires re-heapifying.

One thing at a time. Rather than solve both challenges at once, I’ll first build out support for adding elements. Instead of using polling to verify that adding worked properly, I’ll verify that the priority queue’s underlying heap is valid.

Instead of creating a new test, I introduce a new assertion in each of my two existing tests (abusing the guideline of one assert per test).

   @Test
   public void returnsFIFOWithTwoAlreadyOrderedItems() {
      queue.add(ITEM_100, 100);
      queue.add(ITEM_50, 50);

      assertQueuePollingOrder(ITEM_100, ITEM_50);
      assertValid(queue.heap());
   }

   @Test
   public void returnsInPriorityOrder() {
      queue.add(ITEM_50, 50);
      queue.add(ITEM_100, 100);

      assertQueuePollingOrder(ITEM_100, ITEM_50);
      assertValid(queue.heap());
   }

   private void assertValid(List<PriorityQueue.Node> heap) {
      assertSubheap(heap, Integer.MAX_VALUE, 0);
   }

   private void assertSubheap(
         List<PriorityQueue.Node> heap, int priority, int root) {
      if (root > heap.size() - 1)
         return;

      PriorityQueue.Node node = heap.get(root);
      assertThat(priority > node.priority, is(true));

      assertSubheap(heap, node.priority, leftChild(root));
      assertSubheap(heap, node.priority, rightChild(root));
   }

   private int rightChild(int index) {
      return index * 2 + 2;
   }

   private int leftChild(int index) {
      return index * 2 + 1;
   }

Now I can quickly write the next failing test (with a lame name):

   @Test
   public void heapifiesThreeElements() {
      queue.add(ITEM_50, 50);
      queue.add(ITEM_75, 75);
      queue.add(ITEM_100, 100);

      assertValid(queue.heap());
   }

… and get it to pass quickly too:

   public void add(Object item, int priority) {
      Node node = new Node(item, priority);
      heap.add(node);
      int rightmost = heap.size() - 1;
      if (node.priority > heap.get(0).priority)
         siftUp(rightmost);
   }

   private void siftUp(int rightmost) {
      Node temp = heap.get(0);
      heap.set(0, heap.get(rightmost));
      heap.set(rightmost, temp);
   }

Another test:

   @Test
   public void heapifiesMultipleLevels() {
      add(50, 75, 100, 125);
      assertValid(queue.heap());
   }

This time it requires a bit more manipulation of existing code. I rename siftUp to simply swap, because that’s all it does, and the “sift up” algorithm is in the body of the add method.

   public void add(Object item, int priority) {
      heap.add(new Node(item, priority));
      int current = heap.size() - 1;
      while (priority > parentPriority(current)) {
         swap(current, parent(current));
         current = parent(current);
      }
   }

   private int parentPriority(int index) {
      return heap.get(parent(index)).priority;
   }

   private int parent(int index) {
      return (index - 1) / 2;
   }

   private void swap(int x, int y) {
      Node temp = heap.get(x);
      heap.set(x, heap.get(y));
      heap.set(y, temp);
   }

It’s not bad, but I want each method to be at one level of abstraction, so I modify the add method:

   public void add(Object item, int priority) {
      addNewNode(item, priority);
      siftUpFromLast(priority);
   }

   private void addNewNode(Object item, int priority) {
      heap.add(new Node(item, priority));
   }

   private void siftUpFromLast(int priority) {
      int current = heap.size() - 1;
      while (priority > parentPriority(current)) {
         swap(current, parent(current));
         current = parent(current);
      }
   }

I think that’s it for add! To confirm, I use the example from Wikipedia:

   @Test
   public void integrationTest() {
      add(1, 2, 3, 7, 17, 19, 25, 36, 100);
      assertValid(queue.heap());
   }

Oh yeah–all those test lines required to add elements were getting annoying, so I added the following helper in the test (and updated other tests correspondingly):

   private void add(int... priorities) {
      for (int priority: priorities)
         queue.add(String.valueOf(priority), priority);
   }

On to polling. The general algorithm: capture value at front of heap (the highest priority per add‘s post-condition), move the rightmost item to the front, and re-heapify. To re-heapify: compare the item at the root with both its children, swap with the larger if necessary, and re-heapify the subtree.

From a test perspective, all we need to verify is that the highest priority item was returned and that the completeness post-condition held.

   @Test
   public void removeAnswersHighest() {
      add(20, 30, 40, 100, 50, 60);
      assertThat(queue.poll(), is((Object)"100"));
   }
   @Test
   public void removeReheapifies() {
      add(20, 30, 40);
      queue.poll();
      assertValid(queue.heap());
   }

After incrementally introducing changes based on those two test failures, here’s the explicit bit of code that covers lists up to three elements:

   public Object poll() {
      Object result = heap.remove(0).object;
      if (heap.size() == 2) {
         if (heap.get(0).priority < heap.get(1).priority)
            swap(0, 1);
      }
      return result;
   }

The addition of another item generates the next test failure:

   @Test
   public void removeReheapifyMustConsiderBothChildren() {
      add(40, 20, 30, 10);
      queue.poll();
      assertValid(queue.heap());
   }

I use programming by intention to express the algorithm clearly, starting with the poll method:

   public Object poll() {
      if (heap.size() == 1)
         return heap.remove(0).object;

      Object result = heap.get(0).object;
      reheapify();
      return result;
   }

   private void reheapify() {
      moveLastToFirstSlot();
      if (priorityLessThanEitherChild(0))
         swap(0, highestPriorityChild(0));
   }

The reheapify method has the odd side effect of eliminating the first (highest) element by overwriting it. That this is not explicit in the poll method suggests there’s probably a better expression of the entire algorithm.

I flesh out the trivial supporting methods:

   private void moveLastToFirstSlot() {
      Node rightmost = heap.remove(heap.size() - 1);
      heap.set(0, rightmost);
   }
private boolean priorityLessThanEitherChild(int index) {
      int priority = heap.get(index).priority;
      return priority < leftPriority(index) ||
             priority < rightPriority(index);
   }

   private int highestPriorityChild(int index) {
      return leftPriority(index) > rightPriority(index)
             ? leftChild(index)
             : rightChild(index);
   }

   private int rightPriority(int index) {
      return priority(rightChild(index));
   }

   private int leftPriority(int index) {
      return priority(leftChild(index));
   }

   private int priority(int index) {
      if (index > heap.size() - 1)
         return Integer.MIN_VALUE;
      return heap.get(index).priority;
   }

   private int leftChild(int index) {
      return index * 2 + 1;
   }

   private int rightChild(int index) {
      return index * 2 + 2;
   }

It’s a good chunk of code, but it still fit comfortably within my five-minute cycle limit.

Hey–I recognize those leftChild and rightChild methods from my test! I’ll go make ‘em static so the test can re-use them.

About the only other interesting logic is rooted in the priority method, which returns Integer.MIN_VALUE if there’s no item at an index. That allows comparisons in priorityLessThanEitherChild to quietly fail when no child exists.

Almost there.  The final push is to force repeated swaps from root down through multiple subtrees:

   @Test
   public void removeMustReheapifyMultipleLevels() {
      add(50, 40, 20, 30, 10);
      queue.poll();
      assertValid(queue.heap());
   }

Getting a passing test requires a nice, small increment of code (and a TPP transformation from if=>while. Recursion is one up in the priority list–should I refactor this method?).

   private void reheapify() {
      moveLastToFirstSlot();
      int i = 0;
      while (priorityLessThanEitherChild(i)) {
         int child = highestPriorityChild(i);
         swap(i, child);
         i = child;
      }
   }

Both add and poll methods appear to be complete. Here’s a passing test that fires end-to-end:

   @Test
   public void pollsInPriorityOrder() {
      add(40, 20, 60, 80, 10, 30, 90, 70, 50);
      assertQueuePollingOrder(
         "90", "80", "70", "60", "50", "40", "30", "20", "10");
   }

Boy, do those quotes suck. Well, it’s time to do some global clean-up, so I’ll fix that along with some other things. Here’s the test class after some various cleanup for expressiveness:

import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.*;
import java.util.*;
import org.junit.*;

public class PriorityQueueTest {
   PriorityQueue queue;

   @Before
   public void create() {
      queue = new PriorityQueue();
   }

   @Test
   public void emptyWhenCreated() {
      assertThat(queue.isEmpty(), is(true));
   }

   @Test
   public void noLongerEmptyAfterAdd() {
      queue.add("", 50);

      assertThat(queue.isEmpty(), is(false));
   }

   @Test
   public void singletonQueueReturnsSoleItemOnPoll() {
      queue.add("abc", 50);

      assertThat(queue.poll(), is((Object)"abc"));
   }

   @Test
   public void isEmptyAfterSoleItemRemoved() {
      queue.add("", 50);
      queue.poll();

      assertThat(queue.isEmpty(), is(true));
   }

   @Test
   public void returnsFIFOWithTwoAlreadyOrderedItems() {
      add(100, 50);
      assertQueuePollingOrder(100, 50);
      assertValid(queue.heap());
   }

   @Test
   public void returnsTwoUnorderedItemsInPriorityOrder() {
      add(50, 100);
      assertQueuePollingOrder(100, 50);
      assertValid(queue.heap());
   }

   @Test
   public void heapifiesThreeItems() {
      add(50, 75, 100);
      assertValid(queue.heap());
   }

   @Test
   public void heapifiesMultipleLevels() {
      add(50, 75, 100, 125);
      assertValid(queue.heap());
   }

   @Test
   public void removeAnswersHighest() {
      add(20, 30, 40, 100, 50, 60);
      assertThat(queue.poll(), is((Object)"100"));
   }

   @Test
   public void removeReheapifiesTwoItems() {
      add(20, 30, 40);
      queue.poll();
      assertValid(queue.heap());
   }

   @Test
   public void removeReheapifyMustConsiderBothChildren() {
      add(40, 20, 30, 10);
      queue.poll();
      assertValid(queue.heap());
   }

   @Test
   public void removeMustReheapifyMultipleLevels() {
      add(50, 40, 20, 30, 10);
      queue.poll();
      assertValid(queue.heap());
   }

   @Test
   public void pollExhaustsHeapInPriorityOrder() {
      add(40, 20, 60, 80, 10, 30, 90, 70, 50);
      assertQueuePollingOrder(90, 80, 70, 60, 50, 40, 30, 20, 10);
   }

   private void assertQueuePollingOrder(Integer... expectedItems) {
      List<Integer> items = new ArrayList<Integer>();
      while (!queue.isEmpty())
         items.add(Integer.parseInt((String)queue.poll()));
      assertThat(items, is(Arrays.asList(expectedItems)));
   }

   private void add(int... priorities) {
      for (int priority: priorities)
         queue.add(String.valueOf(priority), priority);
   }

   private void assertValid(List<PriorityQueue.Node> heap) {
      assertSubheap(heap, Integer.MAX_VALUE, 0);
   }

   private void assertSubheap(List<PriorityQueue.Node> heap, int priority, int root) {
      if (root > heap.size() - 1)
         return;

      PriorityQueue.Node node = heap.get(root);
      assertThat(priority > node.priority, is(true));

      assertSubheap(
         heap, node.priority, PriorityQueue.leftChild(root));
      assertSubheap(
         heap, node.priority, PriorityQueue.rightChild(root));
   }
}

And here’s the implementation:

import java.util.*;

public class PriorityQueue {
   private List<Node> heap = new ArrayList<Node>();

   static class Node {
      final Object object;
      final int priority;

      Node(Object object, int priority) {
         this.object = object;
         this.priority = priority;
      }

      public String toString() {
         return String.valueOf(priority);
      }
   }

   public boolean isEmpty() {
      return heap.isEmpty();
   }

   public void add(Object item, int priority) {
      addNewNode(item, priority);
      siftUpFromLast(priority);
   }

   private void addNewNode(Object item, int priority) {
      heap.add(new Node(item, priority));
   }

   private void siftUpFromLast(int priority) {
      int current = heap.size() - 1;
      while (priority > parentPriority(current)) {
         swap(current, parent(current));
         current = parent(current);
      }
   }

   private int parentPriority(int index) {
      return heap.get(parent(index)).priority;
   }

   private int parent(int index) {
      return (index - 1) / 2;
   }

   private void swap(int x, int y) {
      Node temp = heap.get(x);
      heap.set(x, heap.get(y));
      heap.set(y, temp);
   }

   public Object poll() {
      if (heap.size() == 1)
         return heap.remove(0).object;

      Object result = heap.get(0).object;
      reheapify();
      return result;
   }

   private void reheapify() {
      moveLastToFirstSlot();
      int i = 0;
      while (priorityLessThanEitherChild(i)) {
         int child = highestPriorityChild(i);
         swap(i, child);
         i = child;
      }
   }

   private void moveLastToFirstSlot() {
      Node rightmost = heap.remove(heap.size() - 1);
      heap.set(0, rightmost);
   }

   private boolean priorityLessThanEitherChild(int index) {
      int priority = heap.get(index).priority;
      return priority < leftPriority(index)
          || priority < rightPriority(index);
   }

   private int highestPriorityChild(int index) {
      return leftPriority(index) > rightPriority(index)
         ? PriorityQueue.leftChild(index)
         : PriorityQueue.rightChild(index);
   }

   int rightPriority(int index) {
      return priority(rightChild(index));
   }

   int leftPriority(int index) {
      return priority(leftChild(index));
   }

   private int priority(int index) {
      if (index > heap.size() - 1)
         return Integer.MIN_VALUE;
      return heap.get(index).priority;
   }

   List<Node> heap() {
      return heap;
   }

   static int rightChild(int index) {
      return index * 2 + 2;
   }

   static int leftChild(int index) {
      return index * 2 + 1;
   }
}

What else is there to complain about?

  • My test names could use some work. It’s hard to derive good test names when you’re driving incremental implementation of an algorithm, instead of an interface.
  • I probably have some tests I can get rid of.
  • The tests could be composed better. Worse, the test knows about the Node class (which the client need not know about). Maybe binary heap verification should be built into the heap implementation itself.
  • Should there be a PriorityQueue class and a separate Heap class? I thought about this a while and didn’t see the value.
  • I don’t use the Comparable interface to “simplify” and externalize comparisons (i.e. allow the client to specify how priorities compare).
  • I don’t have any tests that show what happens when priorities are equal.
  • I don’t use safety braces (i.e. braces around single-line if statements)–that’s why I have tests. I don’t make everything private that could be private (package access is fine by me, particularly in tests since I don’t have to type the access modifier).

What do I like?

  • I was able to test-drive the implementation, sticking to the five-minute rule for getting green bars. Things were a bit close a couple times (was there a more incremental way?).
  • I think the algorithm as implemented is very easy to follow (read each of the add and poll methods), particularly when compared to what is typically built (here’s one implementation; see, for example, the percolateDown method).

What do you think? Is test-driving a priority queue a good kata? I lay down the challenge flag for a better progression and implementation (shouldn’t be hard).

Posted in coding, kata, TDD, test-driven development, unit testing | Leave a comment

Don’t Stop Reading…

When writing, be careful of presenting an opposing point first with the notion that you’ll make your counterpoint in a subsequent paragraph or page. That first pitch is often as far as some readers get. Royce’s article “Managing the Development of Large Software Systems” led to decades of waterfall for this reason.

Recently, a pair-developer expressed concern over the use of multiple returns in a short method. I said I knew about the long-standing rule but that it created little value for very short methods. Two days later, I heard the same concern during another pairing session. Twice in one week? Curious! I asked where they’d read about the rule. “Uncle Bob’s Clean Code” was the surprising response. “Hand me your copy!” I insisted, a bit worried, and quickly located the text in the functions chapter. The very last paragraph on page 48, starting a new section, states: “Dijkstra rules of structured programming… [say] …every function … should have one entry and one exit. Following these rules means that there should only be one return statement in a function…” Oh! Yeah, I learned that 30 years ago. Really, Bob?

Page 49, top paragraph: “”While we are sympathetic to the goals and disciplines of structured programming, these rules serve little benefit when functions are very small. It is only in larger functions that such rules provide significant benefits.”

Oh, ok. Whew! Crisis of faith averted.

Readers: Inattentive reading can be embarrassing!
Writers: Don’t assume readers will get to your point. Make it first.

Posted in coding, pair programming, pairing, writing | 2 Comments

Not Giving Up on vi(m)

I wrote this blog as a reaction to reading James McKay’s blog entry, “:q!,” in which he describes abandoning vim. A story familiar to me, but I fortunately never abandoned it for good. I do have to agree that Intellisense and other wonders of modern IDEs make them the preferred environment for things like Java, but I still get a lot of use out of vim.

In the past, I’d be forced to use vim while, say, having to do a small amount of my work on Solaris servers, but then I’d be back on Windows shortly thereafter. I loaded RedHat at home many years ago but it didn’t stick. Then I tried Ubuntu… a couple times, and Un*x finally stuck. Yet I was still only occasionally using vim, which meant I was a perpetual newb, at a “level 1″ of vim capability (I’ll sadly call the scale the VCM, or vim capability model).

VCM level 1 is the ability to get in and out of the tool and do basic editing (search, page forward, change/replace text, occasional use of the dot operator, etc.). I remember that I too thought the only way to delete many lines was via count-based commands (e.g., 10d). At VCM level 1, vim beeps at you a lot, and you don’t understand why people swear by it.

What changed things was a result of being forced into having to hit vim on a daily basis for an extended period of time. I had the opportunity to work with Tim Ottinger a bit, and we would pair from time to time. Tim is the author of a great article on vim, “Use vim Like a Pro.” I won’t mention pairing after this paragraph, but I will say that without the ability to pair with someone at a higher VCM level, I might never have advanced much from my level 1 proficiency.

I’m making up the VCM based on my history, of course, but it might just work for others, too. VCM level 2 is marked by use of these critical elements of vim:

  • regular use of the dot operator
  • use of hjkl for cursor movement
  • ability to manage buffers, registers, and split windows
  • ability to visually mark code
  • regular use of f and t in combination with other commands; use of other context-based movement commands
  • use of ctrl-n for auto completion
  • awareness and occasional use of macros
  • occasional use of help facility to learn something new
  • regular (no pun intended) use of regex
  • use of ctags to aid code navigation

Reaching level 2 was important: At level 1, I simply thought vim was a frustrating, not-very-powerful tool. At level 2, I am fairly effective when editing, and faster at many tasks than most people in their GUI editors. I also see that there is much more to learn and master.

Tim is probably at VCM level 4: mastery of most, if not all, of vim’s features. I suppose that marks Level 3 as when you have ingrained all the major facilities (i.e. most everything that Tim covers in Use vim Like a Pro), are trying to ingrain something new on a regular basis (I’ve now habituated the use of ~ to toggle case, for example), have considerably customized your .vimrc, and are making an active attempt to do everything in the most efficient way possible. I am getting into level 3 now, but I haven’t used vim heavily in a while.

During my fortunate opportunities to learn vim from Tim, I discovered there were a few things about vim that he didn’t know. Shocker! It’s an extensive tool, but I guess what that means is that there’s a VCM level 5. Let’s define that as mastering VCM Level 4 plus everything that Tim doesn’t know now.

Posted in IDEs, pairing, vim | Leave a comment

TDD Kata: Roman number converter

I’ve introduced the whole-number-to-Roman-numeral kata in my test-driven development training, not out of sadistic interest, but because I think it offers some excellent lessons about TDD. I do find it a bit amusing that good developers often struggle badly with the derivation of the algorithm, even after I give them my guidelines for the exercise:

  • Don’t assume that your tests must go in order 1, 2, 3, 4, 5, 6, 7, … Seek the test that would make for the next simplest increment of code. Seek patterns–if you’ve figured out the pattern for 1, 2, 3, then what’s the next series of numbers that follows the same pattern?
  • When you refactor incrementally, make similar code look exactly alike, and then get rid of the duplication.
  • Do not prematurely optimize code. Avoid using code-expressiveness “optimizations:” use while loops and simple decrement constructs instead of for loops and post-decrement operators, and let them stand until your algorithm is complete.

After they complete the exercise, we discuss Uncle Bob’s Transformation Priority Premise (TPP), and I ask them to re-run the exercise for homework (and to get them used to the idea of a kata) with the TPP in mind.

The first time I attempted the Kata some years ago, I got lost in the weeds. I didn’t give up; I scrapped a good amount of code, thought a bit harder about the ordering of tests, and eventually made my way to the finish line. I noted at least a couple insights were required, and came up with the guidelines above as a result in order to help students keep on track.

“Let’s start at the beginning, a very good place to start:”

   @Test
   public void convertIntToRoman() {
      assertThat(convert(1), is("I"));
   }

The implementation is the topmost TPP transform, ({}->nil):

   String convert(int arabic) {
      return null;
   }

Quickly following, the passing implementation (nil->constant):

   String convert(int arabic) {
      return "I";
   }

Moving along to the next test:

assertThat(convert(2), is("II"));

There might be a way to get a higher-level transform, but (unconditional->if) seems simplest to me:

   String convert(int arabic) {
      if (arabic == 2) return "II";
      return "I";
   }

The conversion for 3 indicates a repeating pattern:

   assertThat(convert(3), is("III"));

…which begs for a loop (if->while):

   String convert(int arabic) {
      String roman = "";
      while (arabic-- > 0)
         roman += "I";
      return roman;
   }

Performance geeks: never mind the use of String vs StringBuilder/StringBuffer. I want to get the algorithm working first, and all that rot clutters up the code a bit.

Hmm. I didn’t follow my guideline, and used post-decrement where I probably should have just introduced a separate statement to decrement the arabic variable. (Oh and another sin, I’m manipulating the argument. Ah well.)

Now a bit of thought. One, two, three, ten! No, four sir! No, ten. I have support for I, II, III. If I seek a similar pattern, it’s likely to do the least to my code in terms of transforms required, so what if I head to X, XX, and XXX?

   assertThat(convert(10), is("X"));

I can get that to work with a simple (unconditional->if), but it’s horrid:

   String convert(int arabic) {
      if (arabic == 10) return "X";
      String roman = "";
      while (arabic-- > 0)
         roman += "I";
      return roman;
   }

That’s ok–adding a test for 20:

   assertThat(convert(20), is("XX"));

…starts to make things look better, as I introduce (if->while) again:

   String convert(int arabic) {
      String roman = "";
      while (arabic >= 10) {
         roman += "X";
         arabic -= 10;
      }
      while (arabic-- > 0)
         roman += "I";
      return roman;
   }

Those two while loops look an awful lot alike. I can make them look almost exactly alike–I replace my post-decrement (see, shouldn’t have introduced it yet) with a separate statement at the end of the loop body, and adjust my comparison expression:

  String convert(int arabic) {
      String roman = "";
      while (arabic >= 10) {
         roman += "X";
         arabic -= 10;
      }
      while (arabic >= 1) {
         roman += "I";
         arabic -= 1;
      }
      return roman;
   }

It’s clear that these while loops differ only by their data. A bit of insight suggests introducing a table-lookup, allowing the implementation to be factored to the following:

   static int[] ARABIC_DIGITS = { 10, 1 };
   static String[] ROMAN_DIGITS = { "X", "I" };

   String convert(int arabic) {
      String roman = "";
      for (int i = 0; i < ARABIC_DIGITS.length; i++) {
         while (arabic >= ARABIC_DIGITS[i]) {
            roman += ROMAN_DIGITS[i];
            arabic -= ARABIC_DIGITS[i];
         }
      }
      return roman;
   }

(In Java, I tried once building the above as a map, but since the algorithm demands traversing the digits in reverse order, it ends up being a good deal more code than the simple but slightly obnoxious paired arrays.)

Huh. Look at that. A test for 30 should pass too, and it does.

But is it a bit too much implementation? Maybe–now that I support I’s and X’s, I want to be able to combine them, yet it looks like the following tests will pass right off the bat:

   assertThat(convert(11), is("XI"));
   assertThat(convert(33), is("XXXIII"));

And they do! I wonder if there’s a way I could have coded so those tests failed first, but I’ve not thought about it much. Maybe I could have put the second loop in an else block. Ideas?

What about 5?

  assertThat(convert(5), is("V"));

It’s just another digit; adding support for 5 to the table gets this test to pass:

   static int[] ARABIC_DIGITS = { 10, 5, 1 };
   static String[] ROMAN_DIGITS = { "X", "V", "I" };

And now these little combinations involving 5 also pass:

   assertThat(convert(8), is("VIII"));
   assertThat(convert(27), is("XXVII"));

I have to imagine similar tests involving L, C, D, and M should all pass too.

Instead, I’ll hit that pesky one, 4:

   assertThat(convert(4), is("IV"));

The joy of four is that if I try to get there with subtraction, I’m in a world of hurt. This is where students usually go really, really wrong. But, wait… insight! What if I consider that “IV” is just another digit, like any other Roman digit, except that it requires two characters instead of one to represent?

   static int[] ARABIC_DIGITS = { 10, 5, 4, 1 };
   static String[] ROMAN_DIGITS = { "X", "V", "IV", "I" };

Voila. At this point, I’ve covered the various scenarios, now it’s just a matter of fleshing out the table with support for all of the Roman digits. How about I just write a couple tests that fire across all cylinders?

      assertThat(convert(2499), is("MMCDXCIX"));
      assertThat(convert(3949), is("MMMCMXLIX"));

The final “table:”

static int[] ARABIC_DIGITS =
{ 1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1 };
static String[] ROMAN_DIGITS =
{ "M","CM","D","CD","C","XC","L","XL","X","IX","V","IV","I" };

We haven’t had to touch the algorithm for a while. It still holds up!

   String convert(int arabic) {
      String roman = "";
      for (int i = 0; i < ARABIC_DIGITS.length; i++) {
         while (arabic >= ARABIC_DIGITS[i]) {
            roman += ROMAN_DIGITS[i];
            arabic -= ARABIC_DIGITS[i];
         }
      }
      return roman;
   }

I did a search for code that solves this challenge, and had to laugh at the ugliness of a number of the implementations. There were some clever but convoluted ones, some awful ones, and a few that had the same implementation here.

TDD can’t derive algorithms, so they say. I take a different view. An algorithm is simply a “step-by-step procedure for calculations,” per Wikipedia. And TDD is great at helping drive out the vast majority of step-by-step procedures.

Oh–they mean complex algorithms. I see. That’s ok: Most of what we must implement on a day-to-day basis can be done with reasonably simple algorithms. In fact, I’d warrant that far less than a percent of any given system requires anything but simple algorithms that any journeyman programmer can easily devise. (And I’ve seen some very “complex” systems, such as BaBar and the Sabre GDS.)

Complex algorithms do require leaps achieved often only by appropriate insights, sometimes gained only after extensive thought about the problem, and sometimes never reached by a given single mind. TDD will not necessarily help your mind achieve those insights. However, its incremental nature can in some cases give you enough pause and isolation on a small piece of the problem, to the point where the insight needed almost screams at you.

Posted in algorithms, kata, TDD, test-driven development | Leave a comment

Unit Tests Are Still a Waste of Time

Per a recent Dr. Dobbs article, the only reason to write unit tests is as a “convenience … to track down bugs faster.” If that’s the only value proposition, then I must agree with the author that writing unit tests is a “luxury” that only infrequently makes sense.

You’d be far better off investing the time in improving your integration tests or (much better) bolstering the tests that demonstrate customer acceptance criteria. They run much more slowly, but provide more value than writing unit tests after you code: They not only help prevent you from creating defects in the system, they can also be a key negotiation point, and they can act as living documentation on the behaviors your system supports.

Fortunately, writing tests first can create myriad other benefits that outweigh their cost: reduced system size, tests that improve developer understanding of system behaviors, more decoupled/cohesive designs, cleaner code, and the ability to change code safely and rapidly as it needs to be changed. Yes, there’s some churn along the way, but it’s usually mild (and milder as the quality of design improves). These benefits are why I’ve done TDD for a dozen years now, having witnessed some fantastic successes along the way.

Posted in acceptance tests, TAD, TDD, test-driven development, unit testing | Leave a comment

Agile 2011: Live Speaker Feedback

I delivered a talk on “The Only Agile Tools You’ll Ever Need” at Agile 2011, and had a great time doing so. Most of the written feedback from the session indicated that people were very happy with the talk. Negative feedback, received from a handful or so of the ~60 attendees: It didn’t necessarily meet expectations based on what the session summary said. I apologize for that–I’d prepared the summary months before putting together the talk, and what I’d felt important to say changed during that time.

I also muffed a bit–I made a forward reference to the notion that task tracking is a smell, but didn’t quite fully close that thought out as I talked about limiting work in process. If it wasn’t clear, the key point was: if you minimize work in process, the need for tracking tasks in a software tool diminishes significantly.

I handed out index cards at the outset of the session, with the intent of gathering names so I could give out a couple copies of Agile in a Flash. But that’s boring! What else could I have people do with the cards (to bolster my contention that they’re wonderful tools)?

Aha. I gave the following instructions:

“I’m looking to get live feedback during my session. On one side of the index card, draw a big fat smiley face. If you’re happy with what I’m saying, or you think I’m making an astute point, hold up the smiley face. On the other side, let’s see. If you think the believability of what I’m saying is suspect, put something down to represent that. Hmm. Believability Suspect, just abbreviate that, and write down ‘BS‘ in big fat letters. Finally, put your name below the smiley face for the drawing.”

I got a number of smiley faces throughout, but no BS cards. One guy looked like he was about to hold up a BS sign, but changed his mind–probably the one guy who hated the talk. So, my takeaway is that the silly mechanism worked in terms of getting positive feedback–but I suspect that it’s probably a bit too intimidating for most people to challenge a speaker with negative feedback.

Posted in agile, agile in a flash, index cards, speaking | 4 Comments

Buying Back My Soul

I recently traded away my beliefs for security and comfort. I had taken a three-month contract-to-hire at a local software company.

What had I hoped to gain?

  • The company is three miles away from home, a commute of 6 minutes with only one traffic light on the way.
  • They are stable. I received an offer at the end of three months. Due to the industry (government “defense” software), it’s likely I could have stayed there for many years to come.

What was my alternative?

  • Travel. I ultimately chose to return to a life of sporadic travel, consulting and training for Langr Software Solutions. The work is great–I love what I do and love helping people. But it’s travel. For those who’ve never traveled extensively, it’s entertaining the first couple times out. After that, you quickly realize that living in hotels, suffering the hell of airline travel, eating in often-lame restaurants, and being away from home and family is simply not fun at all.

So what was wrong with the local gig?

  • Money. I’m not all about money, and we don’t live extravagantly, but this was the second time I’d taken a pay cut in two years. It’s always tough to cut back on your lifestyle.
  • Industry. I’m a realist, so I understand that “defense” work is necessary, but I’d just as soon not be part of it.
  • Process. I thought I’d be ok with skipping process for a while (they had close to zero process elements). I’d hoped to help introduce some useful techniques and ideas, but it quickly became clear that the culture wasn’t going to support it. They had been reasonably successful without any real process. Not to say that they couldn’t have taken their game to the next level, but one individual can’t change a multi-hundred-person company with zero support.
  • Culture. The folks were nice enough, but I didn’t feel like I was a part of anything. Developers tended to stay in their cubes and keep to their existing cliques. I worked on a two-man development effort (fun stuff; I learned a good amount of Flex/ActionScript). I saw my team member and my boss (both good guys) a few times a week each. The remaining 97%+ of the week, I had close to no additional human interaction.

I sold my soul. Granted, I got to build software, something I love to do, but it was a lonely existence. Had I felt like I was part of a team, as opposed to a bunch of individuals who just happen to reside in nearby cubes, I might have stayed.

I’ve bought back my soul. What’s interesting is that I’m still not part of a team–as a consultant, you are an outsider when you’re traveling, and on your own when at home. But the tradeoff is worth it: I get to help others experience the joy of being part of a true, effective team.

Posted in agile, team | Leave a comment

Interviewer Tips

About every other week, Monster or some portal publishes an article on how to interview properly–what kinds of questions to expect, what kinds of attitudes to carry, and so forth. You’ll also see articles on what sorts of questions to ask as an interviewer. What aren’t published as often are suggestions on how to avoid being a bad interviewer.

While I run a consultancy, I’ve held several full time positions intermittently. I believe in order to stay relevant when training or consulting, it helps dramatically to be part of a real team from time to time. I also find it wise to interview at least once a year–even if you like your current job, it’s always a good idea to find out what’s going on out there.

I’ve had some interesting interviews over my career. I’ve done well on most, and botched a handful too. I’ve also been subjected to “interviewer abuse” many a time. I personally have interviewed close to a couple hundred candidates in my career, and have tried hard to avoid making these candidates miserable.

Here is a short, not exhaustive, list of recommendations I wish my prior interviewers had read before interviewing me:

  • Don’t treat the interview as an opportunity to blather endlessly and show off your skills. Your candidate should be talking at least as much as you.
  • Don’t ask your candidate to “write code” at the whiteboard. Chances are you have at least one computer in your office that the candidate could use.
  • Don’t fixate on overly clever algorithms or puzzles. Insights needed to solve these may not come, particularly if it’s 3:30pm on a Friday and you’ve already subjected the candidate to hours of interviews. Better: Start with a reasonable problem, keep the discussions open-ended, and be willing to shift gears based on what you learn from the candidate.
  • If you think a candidate’s answer to a technical question is wrong, discuss it. There are often other ways to answer a question, and you might have also not done a good job of asking it. Initiate a discussion. You might even learn something yourself.
  • Don’t insist on any more than a half day’s interview. We all know that the important impressions are made or not in the first five minutes, so demanding significantly more time is inconsiderate and suggests vast room for improvement at interviewing.
  • Provide the candidate with a rough agenda, making sure to include what roles each interviewer has in the organization.
  • Ensure your candidate gets exposed to the culture of your company–make sure the interview process accurately reflects it! Make sure they get to see the team at work.
  • Don’t pigeonhole your candidate. Try having an open discussion and see where things lead, instead of forcing him or her through a predetermined interview sequence.
  • Don’t make a candidate slog through the interview process if it’s clear to all parties that he or she is not a good fit. If you think there’s a problem, have the courage to initiate a dialog with the candidate about the supposed mismatch. Ask them where their strengths lie, and look to take the discussions there. Chances are that you’re not doing a good enough job of asking questions.
  • Don’t deliberately make your candidate uncomfortable. Avoid intimidating behavior, such as staring angrily or morosely and refusing to utter more than a syllable or two. Don’t forget to ask them if they need water or a restroom break.
  • If you insist on “behavioral interviewing,” make it clear to the candidate how his or her answers correspond to your decisions.
  • If you are declining to extend an offer to the candidate, be honest with them about the reasons why.
  • Thank your candidate by phone or email within a day of the interview, and indicate when you will have an answer.
Posted in interviewing | 1 Comment

Pitting Teams Against One Another

I was asked this via email:

I just read your article

http://www.developer.com/tech/article.php/3715196

And, I came across this principle:
Don’t use metrics to compare teams. Metrics are best viewed as probes and indicators of problem areas that may warrant further investigation.

Interestingly, this is one of the drivers that my employer wants to collect metrics.

My responses:

Simply put, no two teams are alike.

From another perspective, comparing two points completely misses the goal of doing agile software development. A good agile team is continually looking for ways to improve their productivity–i.e. within the team itself. Most teams have massive potential room for improvement, never mind worrying about what the other teams are up to. They don’t need distraction and concern about keeping their jobs–what they need instead is good leadership and support.

Competition in the marketplace can be a great way to end up with better product, but creating competition between two teams producing different products is artificial and pointless.

There’s also no “fair” way to compare two teams working on different products. Many factors can be a reason why any one team has a significant advantage or disadvantage over other: difficult vs rapid technology (e.g. C++ vs Ruby), team size, geographical considerations, quality of existing code, weak team members, duration together working as a team, bad managers, fire drills, crankier customers, and so on. How do you provide a good measurement of productivity based on all these factors?

You might come up with something that seems reasonably fair, but you’re better off just focusing on getting each team to use their own past productivity as a baseline. The only realistic measurements are relative (and even those are skewed by things such as departing team members).

It can also be counter-productive to focus on racing other teams: What if one team finishes 50% more stories but delivers 20% more defects, resulting in a customer threatening to drop your product? What if you worry about technical metrics (such as code coverage), and one team gets high coverage but their tests stink (seen this–it’s subjective, and easy to tell for a seasoned TDD developer but not anyone else)?

If management focuses on a small number of metrics, and mandates certain goals for them, you can guarantee that the smarter teams will do what they can to play the numbers game. More often than not, a singular metric will have little to do with legitimate goals (customer satisfaction, on-time delivery, high quality). Due to management mandate, I saw a team rapidly increase their code coverage by rapidly cutting and pasting tests, producing such low quality tests that they eventually abandoned. The end result was a complete waste of investment in the unit tests, because the team wasn’t concerned about what really mattered.

Short-term goals may make the numbers look good, but in the long run you will pay for the misguided focus.

Worse, it can be demoralizing. I worked in a large Fortune 500 company where a VP with seemingly good intentions pitted teams against one another by issuing letter grades (A, B, C, D, F). He insisted on having a meeting with each of 30+ teams in his organization and lambasting those who he perceived to be inadequate. Never mind that there were factors for some of the teams that had nothing to do with team productivity, but instead with customer or internal politics.

The only time I would even consider a team-vs-team competition is if both were high-perfoming, kicking-rear teams. That might be fun. But it’s hard enough to find one top-notch team, and almost impossible to find two in the same place.

Posted in agile, metrics | Leave a comment