Monday, March 19, 2012

Agile programming in extremis -- the final episode.

See the previous post (Agile programming in extremis) for the start of the story.

The code needed one final refactoring to reach it's ultimate state. (8 files, 64 lines of production code. 160 lines of test code (yes, the test got smaller!) The observation that triggered this one was, “Some of the tests depend on other objects working correctly. The test for each object should stand alone.”
public class EventListener{
  private EventComparer eventComparer;
  private DuplicateEventHandler duplicateEventHandler;
  private Event previousEvent = null;
  
  public EventListener(
      DuplicateEventHandler duplicateEventHandler){
    this.(duplicateEventHandler, new EventComparer());
  }
  
  EventListener(
       DuplicateEventHandler duplicateEventHandler, 
       EventComparer eventComparer)
    this.duplicateEventHandler = duplicateEventHandler;
    this.eventComparer = eventComparer;
  }
  
  public void onEvent(Event currentEvent)
  {
    bool previousEventIsValid = (previousEvent != null);
    if(previousEventIsValid && eventComparer.isSameEvent(
         previousEvent, currentEvent)) {
       duplicateEventHandler.handleSameEvent(currentEvent);
    }
    previousEvent = currentEvent.cloneEvent(); 
  }    
};

public class EventComparer{ 
  private EventLocationComparer eventLocationComparer;
  private EventTimeComparer eventTimeComparer;
  
  public EventComparer(){
    this(new EventLocationComparer(), new EventTimeComparer());
  }
  
  EventComparer(
      EventLocationComparer eventLocationComparer, 
      EventTimeComparer eventTImeComparer){
    this.eventLocationComparer = eventLocationComparer;
    this.eventTimeComparer = eventTimeComparer;
  }

  public bool isSameEvent(
          Event currentEvent, Event previousEvent){
    return eventLocationComparer.isSameLocation(
               currentEvent, prevousEvent) &&
           eventTimeComparer.isSameTime(
               currentEvent, previousEvent);
  }  
};

public class EventLocationComparer{
  static final int X_LIMIT = 200;
  static final int Y_LIMIT = 200;
  
  public bool isSameLocation(
        Event currentEvent, Event previousEvent){
    return
     (isSamePoint(currentEvent.getX(), previousEvent.getX(), 
               X_LIMIT) &&
     (isSamePoint(currentEvent.getY(), previousEvent.getY(), 
               Y_LIMIT);
  }
  
  public bool isSamePoint(
       int current, int previous, int tolerance){
    return math.abs(current - previous) < tolerance;
  }
};

public class TimeComparer{
  static final long TIME_LIMIT = 500L;

  public bool isSameTime(
       Event currentEvent, Event previousEvent){
    return currentEvent.getTime() - previousEvent.getTime() 
           < TIME_LIMIT;
  }  
};

public class EventListenerTest{
  private EventListener testObject;
  
  @Mock
  private EventComparer eventComparer;
  @Mock
  private Event eventOne;
  @Mock
  private Event eventTwo;
  
  @Before
  public void setup(){
    testObject = new EventListener(
       duplicateEventHandler, eventComparer);
  }    

  @Test  
  public void whenEventComparerReturnsTrueHandleSameEvent(){
    when(eventComparer.isSameEvent).
         withArguments(eventOne, eventTwo).
         thenReturn(true);
    
    testObject.onEvent(eventOne);
    testObject.onEvent(eventTwo);
    
    verify(testObject).handleSameEvent());
  }

  @Test  
  public void whenEventComparerReturnsFalseDoNotHandleSameEvent(){
    when(eventComparer.isSameEvent).
          withArguments(eventOne, eventTwo).
          thenReturn(false);
    
    testObject.onEvent(eventOne);
    testObject.onEvent(eventTwo);
    
    verify(testObject,never()).handleSameEvent());
  }
};

public class EventComparerTest{
  private EventComparer testObject;
  
  @Mock
  private EventLocationComparer eventLocationComparer;
  @Mock
  private EventTimeComparer eventTimeComparer;
  @Mock
  private Event eventOne;
  @Mock
  private Event eventTwo;

  @Before
  public void setup(){
    testObject = new EventComparer(
        eventLocationComparer, eventTimeComparer);
  }

  @Test
  public void testIfSameLocationAndSameTimeThenReturnTrue(){
    when(EventLocationComparer.isSameLocation).
         withArguments(eventOne, eventTwo).
         thenReturn(true);
    when(EventTimeComparer.isSameTime).
         withArguments(eventOne, eventTwo).
         thenReturn(true);
    
    assertTrue(testObject(eventOne, eventTwo));
  }
  // additonal tests for the other cases go here.
}

public class EventLocationComparerTest{
  private EventLocationComparer testObject;
  
  private Event event1 = new Event(parameter);

 @Before
  public void setup(){
    testObject = new EventLocationComparer();
  }
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters
        to test boundary conditions);

    bool testResult = testObject.isSameLocation(test1, test2);
    
    assertTrue(testResult);
  }
  
  @Test
  public void thisIsAnotherTypicalTest(){
    Random random = new Random();
    int testValue1 = random.next();
    int testLimit = random.next();
    int testValue2 = testValue1 
              plus or minus testLimit 
              plus or minus one.
    
    bool testResult = testObject.isSamePoint(
          testValue1, testValue2, testLimit);
    
    assertTrue(testResult);    
  }
};

public class EventTimeComparerTest{
  private EventTimeComparer testObject;
  
 @Before
  public void setup(){
    testObject = new EventTimeComparer();
  }
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters
          to test boundary conditions);

    bool testResult = testObject.isSameTime(
       test1, test2);
    
    assertTrue(testResult);
  }
};
This is not a criticism of Agile methodologies in general. It's just an observation that moderation and common sense can be appropriate in Agile development, too. There must be a happy-state somewhere between the first and the last versions of this code. I'll mention one more interesting point. After all the above work, the system still had a major bug (a show-stopper as it turns out). The "handleSameEvent method which is not shown here, was just as thoroughly tested. Alas, even though it passed the tests, it did not, in fact, do the right thing. One wonders (OK, I wonder) if the time that was invested in rewriting and re-rewriting this working and tested code had been invested in diagnosing the real problem whether the system as a whole might be in better shape.

Agile programming in extremis -- third refactoring's a charm.

See the previous post (Agile programming in extremis) for the start of the story.

The problem with the code from the previous post is "All those private methods are untestable." To resolve this we refactor again -- ending up with 8 files containing 50 lines of production code and approximately 180 lines of test code, like so:
public class EventListener{
  private EventComparer eventComparer = new EventComparer();
  private DuplicateEventHandler duplicateEventHandler;
  private Event previousEvent = null;
  
  public EventListener(
      DuplicateEventHandler duplicateEventHandler){
    this.duplicateEventHandler = duplicateEventHandler;
  }
  
  public void onEvent(Event currentEvent)
  {
    bool previousEventIsValid = (previousEvent != null);
    if(previousEventIsValid 
         && eventComparer.isSameEvent(
              previousEvent, currentEvent)) {
       duplicateEventHandler.handleSameEvent(currentEvent);
    }
    previousEvent = currentEvent.cloneEvent(); 
  }    
};

public class EventComparer{ 
  private EventLocationComparer eventLocationComparer 
        = new EventLocationComparer();
  private EventTimeComparer eventTimeComparer = 
           new EventTimeComparer();

  public bool sameEvent(
       Event currentEvent, Event previousEvent){
    return 
     eventLocationComparer.isSameLocation(
           currentEvent, prevousEvent) &&
     eventTimeComparer.isSameTime(
           currentEvent, previousEvent);
  }  
};

public class LocationComparer{
  static final int X_LIMIT = 200;
  static final int Y_LIMIT = 200;

  public bool isSameLocation(
        Event currentEvent, Event previousEvent){
    return (isSamePoint(
                currentEvent.getX(), 
                previousEvent.getX(), 
                X_LIMIT) &&
           (isSamePoint(
                currentEvent.getY(), 
                previousEvent.getY(), 
                Y_LIMIT);
  }
  
  public bool isSamePoint(
      int current, int previous, int tolerance){
    return math.abs(current - previous) < tolerance;
  }
};

public class TimeComparer{
  static final long TIME_LIMIT = 500L;

  public bool isSameTime(
       Event currentEvent, Event previousEvent){
    return currentEvent.getTime() - previousEvent.getTime() 
        < TIME_LIMIT;
  }  
};

public class EventListenerTest{
  private EventListener testObject;
  
  private Event event1 = new Event(parameter);

  @Mock
  private DuplicateEventHandler duplicateEventHandler;

  @Before  
  public void setup(){
    testObject = new EventListener(duplicateEventHandler);
    testObject.onEvent(event1);
  }    
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters
       to test boundary conditions);
    testObject.onEvent(event2);
    verify(duplicateEventHandler).handleSameEvent(event2);
    // or depending on the parameters to event2...
    verify(duplicateEventHandler, never()).
       handleSameEvent(event2);
  }
};

public class EventComparerTest{
  private EventComparer testObject;

  private Event event1 = new Event(parameter);

 @Before
  public void setup(){
    testObject = new EventComparer();
  }  
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters 
       to test boundary conditions);

    bool testResult = testObject.sameEvent(test1, test2);
    
    assertTrue(testResult);
  } 
};

public class EventLocationComparerTest{
  private EventLocationComparer testObject;
  
  private Event event1 = new Event(parameter);

 @Before
  public void setup(){
    testObject = new EventLocationComparer();
  }
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters 
       to test boundary conditions);

    bool testResult = testObject.isSameLocation(
          test1, test2);
    
    assertTrue(testResult);
  }
  
  @Test
  public void thisIsAnotherTypicalTest(){
    Random random = new Random();
    int testValue1 = random.next();
    int testLimit = random.next();
    int testValue2 = testValue1 
              plus or minus testLimit plus or minus one.
    
    bool testResult = testObject.isSamePoint(
            testValue1, testValue2, testLimit);
    
    assertTrue(testResult);    
  }
};

public class EventTimeComparerTest{
  private EventTimeComparer testObject;
  
 @Before
  public void setup(){
    testObject = new EventTimeComparer();
  }
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters 
       to test boundary conditions);

    bool testResult = testObject.isSameTime(test1, test2);
    
    assertTrue(testResult);
  }
};
It may be a bit long, but hey, it's testable. (..or not. See the next post.)

Agile Programming in extremis -- re-refactoring.

See the previous post (Agile programming in extremis) for the start of the story.

The next person to look at the code said: "What does isSameEvent do? I can't tell from the code." The changes resulted in two files containing 39 lines of production code and 50 lines of test code.
public class EventListener{
  static final int X_LIMIT = 200;
  static final int Y_LIMIT = 200;
  static final long TIME_LIMIT = 500L;

  private DuplicateEventHandler duplicateEventHandler;
  private Event previousEvent = null;
  
  public EventListener(
      DuplicateEventHandler duplicateEventHandler){
    this.duplicateEventHandler = duplicateEventHandler;
  }
  
  public void onEvent(Event currentEvent)
  {
    bool previousEventIsValid = (previousEvent != null);
    if(previousEventIsValid 
         && isSameEvent(previousEvent, currentEvent)) {
       duplicateEventHandler.handleSameEvent(currentEvent);
    }
    previousEvent = currentEvent.cloneEvent(); 
  }    
  
  private bool isSameEvent(
      Event currentEvent, Event previousEvent){
    return isSameLocation(currentEvent, prevousEvent) &&
           isSameTime(currentEvent, previousEvent);
  }
  
  private bool isSameLocation(
        Event currentEvent, Event previousEvent){
    return (isSamePoint(
               currentEvent.getX(), 
               previousEvent.getX(), 
               X_LIMIT) &&
           (isSamePoint(
               currentEvent.getY(), 
               previousEvent.getY(), 
               Y_LIMIT);
  }
  
  private bool isSameTime(
       Event currentEvent, Event previousEvent){
    return currentEvent.getTime() - previousEvent.getTime() 
      < TIME_LIMIT;
  }
  
  private bool isSamePoint(
       int current, int previous, int tolerance){
    return math.abs(current - previous) < tolerance;
  }
};

public class EventListenerTest{
  private EventListener testObject;
  
  private Event event1 = new Event(parameter);

  @Mock
  private DuplicateEventHandler duplicateEventHandler;

  @Before  
  public void setup(){
    testObject = new EventListener(
         duplicateEventHandler);
    testObject.onEvent(event1);
  }    
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(varying parameters 
       to test boundary conditions);
    testObject.onEvent(event2);
    verify(duplicateEventHandler).handleSameEvent(event2);
    // or depending on the parameters to event2...
    verify(duplicateEventHandler, never()).
         handleSameEvent(event2);
  }
};

Agile programming in extremis -- the first refactoring.

See the previous post (Agile programming in extremis) for the start of the story.

The first agile developer to examine this code said: “It’s not clear what that if statement is testing.” and produced the following improved version (2 files; 28 lines of production code; around 50 lines of test code):
public class EventListener{
  static final int X_LIMIT = 200;
  static final int Y_LIMIT = 200;
  static final long TIME_LIMIT = 500L;

  private DuplicateEventHandler duplicateEventHandler;
  private Event previousEvent = null;
  
  public EventListener(
       DuplicateEventHandler duplicateEventHandler){
    this.duplicateEventHandler = duplicateEventHandler;
  }
  
  public void onEvent(Event currentEvent)
  {
    bool previousEventIsValid = (previousEvent != null);
    if(previousEventIsValid && 
         isSameEvent(previousEvent, currentEvent)) {
       duplicateEventHandler.handleSameEvent(currentEvent);
    }
    previousEvent = currentEvent.cloneEvent(); 
  }    
  
  private bool isSameEvent(
       Event currentEvent, Event previousEvent){
    return 
      ((math.abs(currentEvent.getX() – previousEvent.getX()) 
         < X_LIMIT) &&
       (math.abs(currentEvent.getY() – previousEvent.getY()) 
         < Y_LIMIT) &&
       (currentEvent.getTime – previousEvent.getTime() 
         < TIME_LIMIT));
  }
};

public class EventListenerTest{
  private EventListener testObject;
  
  private Event event1 = new Event(parameter);

  @Mock
  private DuplicateEventHandler duplicateEventHandler;

  @Before  
  public void setup(){
    testObject = new EventListener(duplicateEventHandler);
    testObject.onEvent(event1);
  }    
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters
       to test boundary conditions);
    testObject.onEvent(event2);
    verify(duplicateEventHandler).handleSameEvent(event2);
    // or depending on the parameters to event2...
    verify(duplicateEventHandler, never()).handleSameEvent
         (event2);
  }
};

Agile programming in extremis

I was recently working on a project with a team that had adopted Agile development methods with a vengeance.  In the next few posts, I'll report on the evolution of some code during repeated refactorings.

To protect the innocent and obscure the guilty, I am paraphrasing the actual code and omitted some of the boilerplate and other detail, but rest assured that Dave Barry would agree that "I am not making this up."

To get started, here is the original code.  There are two files. Before I formatted it for this blog it containing 23 lines of production code and around 50 lines of test code (of which 23 are shown here.)

public class EventListener{
  static final int X_LIMIT = 200;
  static final int Y_LIMIT = 200;
  static final long TIME_LIMIT = 500L;

  private final DuplicateEventHandler duplicateEventHandler;
  private Event previousEvent = null;
  
  public EventListener(
         DuplicateEventHandler duplicateEventHandler){
    this.duplicateEventHandler = duplicateEventHandler;
  }
  
  public void onEvent(Event currentEvent)
  {
    if( previousEvent != null &&
       (math.abs(currentEvent.getX() – previousEvent.getX()) 
          < X_LIMIT) &&
       (math.abs(currentEvent.getY() – previousEvent.getY()) 
          < Y_LIMIT) &&
       (currentEvent.getTime – previousEvent.getTime() 
          < TIME_LIMIT)) {
       duplicateEventHandler.handleSameEvent(currentEvent);
    }
    previousEvent = currentEvent.cloneEvent(); 
  }
};

public class EventListenerTest{
  private EventListener testObject;
  
  private Event event1 = new Event(parameter);

  @Mock
  private DuplicateEventHandler duplicateEventHandler;

  @Before  
  public void setup(){
    testObject = new EventListener(duplicateEventHandler);
    testObject.onEvent(event1);
  }    
  
  @Test
  public void thisIsATypicalTest(){
    Event event2 = new Event(various parameters 
           to test boundary conditions);
    testObject.onEvent(event2);
    verify(duplicateEventHandler).handleSameEvent(event2);
    // or depending on the parameters to event2...
    verify(duplicateEventHandler, never()).handleSameEvent(
       event2);
  }
};
If you are a programmer, now would be a good time to examine the code above and decide what changes, if any, you would make to improve it.

Three and a bit years lapse....

I haven't added anything to this blog since January 2009.  Lots has happened, but apparently a facebook status update is sufficient.   Now I have something more meaty to talk about, so....