How a Code Retreat Taught Me to Be Comfortable Throwing Away Code

Can you believe that I passed up a beautiful August weekend, at a cottage on a pristine lake just outside Algonquin Park, to spend the day back in the hot, humid city writing code for 45 minutes at a time and immediately throwing the code away? I can hardly believe it myself.

The code retreat started at 8:30am, bright and early, on Saturday, August 8. The retreat was hosted by Mike DiBernardo at the PlateSpin office and led by Joe (J.B.) Rainsberger and Corey Haines.

Corey needed a place to stay in Toronto so I offered him a place chez moi (lots of room because the rest of my family was, naturally, at the cottage swimming in the pristine lake). Early Saturday morning Corey and I took the Red Rocket from my place in Kensington Market to Platespin.

After some bagels and coffee, Corey and Joe introduced us participants to the structure of the code retreat: work in focused sessions of 45 minutes; use Java because it is a straight forward language that most everyone understands; develop the code in pairs (or triples); switch pairs after each session; pick at least one thing as the learning objective for each session; and, finally, throw the code away after each session.

What? Throw away the code after each session? Delete the code from version control too?

Yep. I found it quite difficult to throw away the first session's code. However, by the end of the code retreat I was happily throwing away my code.

The goal of a code retreat is to practice programming rather than to accomplish a programming task. The structure of the code retreat cleverly supported this goal in the following ways:

1. Starting as 8:30 in the morning on a summer Saturday guarantees only keen participants who wanted to learn;


2. 45 minutes is too short to complete an implementation of Conways's Game of Life;


3. While one participant characterized Java as a “baby” language, I think the choice was a good one because we didn't spend a whole lot of time getting past basic understanding which we might have done had we chosen some other language;


4. Pairing and frequent pair switching are practices that emphasise learning rather than immediate productivity;


5. Because every pair was asked to choose one learning goal for each session the programming in each session felt like deliberate practice;


6. Throwing away the code each time was liberating because I could start a session with a new pair without either one of us having to understand the code that we had just written in the previous session.

Here are some of the deliberate learning goals that my pairs and I chose: don't use if statements, work inside out by implementing the survival rules for a single cell, write tests using mock objects, and work outside in by writing tests using a DSL to describe the transition from one generation to the next.

Most of the code that I wrote with my pair was awkward and not worth keeping. But that is okay. As Chad Fowler writes in his book, The Passionate Programmer, if I sit down to practice coding and nothing but elegant code comes out, I'm probably sitting near the “center” of my current capabilities instead of the edges, where a good practice session should place me.

The code retreat reminded me how important it is to make time for deliberate practice. That is, make time where it is okay to ignore the strong pull of completing tasks and instead give myself a chance to experiment, learn, and practice my craft.

Time to Impose a Requirements Tax On Your Project?

I've been reading and enjoying these beautiful O'Reilly books: Beautiful Architecture, Beautiful Code, and Beautiful Teams. I have yet to read these other titles from the collection: Beautiful Data, Beautiful Security, and Beautiful Testing.

Those books will have to wait because the O'Reilly book that I'm reading right now is Masterminds of Programming. I want to share something that Tom Love wrote on page 248:

I just handed out to some of my friends this week a big pile of buttons with the word REQUIREMENTS and a red slash through it like a European road sign. On the back of the button are 14 acceptable alternatives to that word. Lots of discussions go on between individuals or between groups of the form of "I couldn't do this work because you didn't give me the requirements yet," or "We need to have a group of people that goes out and gathers the requirements for this new system."

The term is simply too imprecise. You need to have more precise terms as an alternative. On a big project that I've been involved in we have imposed a requirements tax. If anybody uses the word "requirements" standalone, they have to add $2.00 to the entertainment fund. If they want to talk about use cases, or if they want to talk about story cards, or they want to talk about performance metrics, or they want to talk about business cases or business process models, those are all acceptable terms. They don't incur a tax, because now if you say, "I need to have the use cases or the functional specification, or a mockup of the application that needs to be developed," that's a precise request.

Let it be known that I will no longer use the word "requirements" standalone. If you catch me, I'll buy you a beer. Now if you use the word "requirements" standalone, I'm likely to just get angry. Dear reader, you have been warned.

What Is That Smell?

Do not add an equals() method to a class to make test writing easier. I did just that a few months ago and have only just discovered the consequences.

I quite like JUnit assertThat() syntax. I wanted to write this test assertion:

assertThat(sut.collapse(rawResults),
  hasItems(expectedBloodPressure_1, expectedBloodPressure_2));

However, the test assertion failed to work when I first executed my test because I needed an equals() method on the Result class that treated Result like a value object. That is, I wanted to test for equality based on the values of all the Result fields. Without thinking through the consequences, I added this method:

public boolean equals(Object obj) {
  return EqualsBuilder.reflectionEquals(this, obj);
}

Nice. Using the Apache Commons EqualsBuilder.reflectionEquals() method was simple and clear.

End of story. New code tested with an expressive and easy to write unit test. Not quite.

In the same application, during an overnight process that synchronizes changes from a data warehouse, new results are extracted, transformed, and loaded into the application.

I was doing some performance analysis to try to speed up this process and I noticed that most of the time was spent talking to the data warehouse. No surprise there. I scanned down to locate the first method that was not in the JDBC driver or in the JDBC library and found the equals() method on Result. It was being called over 700,000 times. What had previously been a super fast address comparison was now a reflection based equals. Yikes.

Clearly, adding an equals() method to make test writing easier was a bad idea. Gerard Meszaros categorizes this smell as a special case of Test Logic in Production that he has named Equality Pollution.

So, what did I learn? One, no test logic in production code. Two, never do performance tuning without a tool that lets you see what is actually going on.

Are You Using Mocks When You Should Be Using Test Data Builders?

I want to show what happens when you use mock objects when you should be using test data builders.

This post is a follow-up to a question that came up during a presentation that Jason and I gave at the March XP Toronto meeting.

We want to test the addMember() method of the CareTeamMembershipService. This method makes a Patient a member of a CareTeam.



As you can see from the implementation below there are two business rules to enforce: (1) patients can only be added to care teams at facilities where the patient is registered and (2) patients have to meet the care team membership criteria.

public void addMember(Patient patient, CareTeam careTeam) {
  if (!patient.isRegisteredAt(careTeam.getFacility())) {
    throw new CareTeamAdminException();
  }
    
  if (!patient.meets(careTeam.getMembershipCriteria())) {
    throw new CareTeamAdminException();
  }
    
  careTeamMembershipDao.create(patient.getId(), careTeam.getId()); 
}

Here is the test for the addMember() method using mock objects and test data builders:

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = aFacility().build();
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

Now that I'm quite used to jMock and test data builder syntax, I find that this test reads like a specification.

During our presentation there was one very interesting question from the audience that was never answered directly: Could you use a Mock instead of a Test Data Builder for the Facility instance?

How about we try right now and see what happens?

Use a mock for the jacobi Facility instance.

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = context.mock(Facility.class);
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

The test fails because Facility is not an interface.

One solution is to create interfaces for all your domain objects. Yech. Alternatively the jMock framework will let you mock concrete and abstract classes although it is discouraged. To enable this feature you have to configure the context in a special way.

Use a ClassImposteriser mockery.

final Mockery context = new JUnit4Mockery() {{
  setImposteriser(ClassImposteriser.INSTANCE); 
}};

Now the test fails because of an unexpected invocation: facility.register().

Add expectations on the facility mock object.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
    
  context.checking(new Expectations() {{
    one(jacobi).register(with(any(Patient.class)));
    one(jacobi).register(with(any(CareTeam.class)));
  }});
    
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
    
  context.checking(new Expectations() {{
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
    
  sut.addMember(patient, careTeam);
}

This fix is not so simple because we have to put both of the register expectations before we've actually created our patient and careTeam. We also have to specify the expected arguments with matchers rather than the actual arguments.

Now the test fails because of an unexpected invocation: facility.getPatients().

Add another expectation on the facility mock object.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
    
  context.checking(new Expectations() {{
    one(jacobi).register(with(any(Patient.class)));
    one(jacobi).register(with(any(CareTeam.class)));
  }});
    
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
    
  context.checking(new Expectations() {{
    one(jacobi).getPatients(); will(returnValue(singleton(patient)));
    one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
    
  sut.addMember(patient, careTeam);
}

Now the test finally passes. However, I find it way less readable as a specification. Why does the specification for the addMember() function require expectations on the Facility instance?

Here is the same test where all the test data builders have been replaced by mock objects.

public void should_permit_add_for_appropriate_care_team() throws Exception {
  final Facility jacobi = context.mock(Facility.class);
  final Patient patient = context.mock(Patient.class);
  final CareTeam careTeam = context.mock(CareTeam.class);
  final MembershipCriteria membershipCriteria = context.mock(
                                  MembershipCriteria.class);
    
  context.checking(new Expectations() {{
    one(patient).getId(); will(returnValue("id1"));
    one(careTeam).getId(); will(returnValue("id2"));
    one(careTeam).getFacility(); will(returnValue(jacobi));
    one(patient).isRegisteredAt(jacobi); will(returnValue(true));
    one(careTeam).getMembershipCriteria(); will(returnValue(membershipCriteria));
    one(patient).meets(membershipCriteria); will(returnValue(true));
    one(careTeamMembershipDao).create("id1", "id2");
  }});
    
  sut.addMember(patient, careTeam);
}

This is what my tests looked like when I used mock objects without test data builders.

I find it really hard to separate the expectations that have to do with the system under test from the expectations that have to do with the data setup. I also have to know lots of details about the internals of the depended-on components: patient, careTeam, membershipCriteria, and facility.

So always use mock objects with test data builders and your tests can look like this:

public void should_permit_add_for_appropriate_care_team() {
  final Facility jacobi = aFacility().build();
  final Patient patient = aPatient().at(jacobi).age(18).with(DIABETES).build();
  final CareTeam careTeam = anAdultCareTeam().at(jacobi).with(DIABETES).build();
 
  context.checking(new Expectations() {{
 one(careTeamMembershipDao).create(patient.getId(), careTeam.getId());
  }});
 
  sut.addMember(patient, careTeam);
}

Sweet.

Presentation at XP Toronto

At the March XP Toronto meeting, Jason Cheong-Kee-You and I gave this presentation, Struggling to Create Maintainable Unit Tests?

At the start of the presentation we asked our audience if they where using:

• a unit testing framework
• any Mock Object framework
• Test Data Builders

We also asked if they had read xUnit Test Patterns.

It turned out that essentially everyone was using a unit testing framework and most everyone in the audience had had some exposure to a mocking framework. Very few people had read xUnit Test Patterns. No one was sure about Test Data Builders.

It was the perfect audience for our presentation.

The takeaway from our presentation is that you need to use mock objects with Test Data Builders if you want to create maintainable unit tests. Use mock objects when protocol is important and use Test Data Builders when data is important.

Here is our call to action:

• read xUnit Test Patterns
• read Nat Pryce's blog postings on Test Data Builders
• learn to distinguish between depended-on components that are essentially protocol versus those that are essentially data
• try using mock object with Test Data Builders.

I Might Be Able to Repair Cars After All

I often tell people that in our modern world software is everywhere. I had no idea how much software was in cars. Here are two quotes from Manfred Broy in IEEE Spectrum:

[A] premium-class automobile ... probably contains close to 100 million lines of software.

The cost of software and electronics can reach 35 to 40 percent of the cost of a car ... with software development contributing about 13 to 15 percent of that cost.

When I first met Su, her Grandmother asked me, "What can you do?" I said I was a programmer. She asked me again, "No. No. What can you do with your hands? What can you make?" From her point of view I was pretty useless. Her husband was a tool and die maker and made all the cupboards in her kitchen. When he got older he made himself a wood splitter that we still use. He could certainly make things.

I never imagined that I would be able to repair a car. I might have been wrong.

Useful Habit: Put the Project and the Customer Ahead of Your Personal Interests

In I. M. Wrights's Hard Code columns, Nailing the Nominals and Green Fields Are Full of Maggots, you will find a case to watch out for the changes that are needed in your development approach when your codebase is greater than 100K LOC. I think he is correct. Of course, Eric knows that there is no hard cutoff between programming in the small and programming in the large.

I'm most interested in developing and teaching those habits that are useful all along the programming in the small to the programming in the large continuum.

Here's a habit for software engineers to start with from Nailing the Nominals: put the project and the customer ahead of your personal interests.

You Should Be Using A Static Analysis Tool

In my experience static analysis tools do not provide very much benefit. Nonetheless, you'd be crazy, dare I say unprofessional, not to be using a static analysis tool.

These days I'm coding in Java and I'm using FindBugs as my static analysis tool. There is almost no cost to installing and using FindBugs. Most importantly, FindBugs doesn't have my particular set of blind spots.

Let's see what FindBugs discovered about my current codebase, a codebase that has been in production for 4 years.

Dodgy 1: Method uses the same code for two branches.

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_YES);

I resolved the FindBugs discovery by replacing the dodgy code with:

selectField.getModel().setSelectedItem(
    theDescriptor.isActive() ? ITEM_ASSESSMENT_YES : ITEM_ASSESSMENT_NO);

This is a real bug discovered by FindBugs. In the dodgy code above, a checkbox in the UI is always selected without considering the value returned from the isActive() method. In my current codebase, FindBugs found three occurrences of the same mistake. No doubt from the essential but sometimes dangerous practice of cut-and-paste coding.

Dodgy 2: Using non-short-circuit logic (e.g., & or |) rather than short-circuit logic (&& or ||).

if (getDiseaseToMeasuresOrder() != null &
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

I resolved the FindBugs discovery by replacing the dodgy code with:

if (getDiseaseToMeasuresOrder() != null &&
    getDiseaseToMeasuresOrder().containsKey(theDisease)) {

This is a real bug discovered by FindBugs. In the dodgy code above, whenever the getDiseaseToMeasuresOrder() method returns null we'll get a NullPointerException. I'm pretty sure this bug was simply due to a typo.

Dodgy 3: Switch statement found where one case falls through to the next case.

<br />AbstractEvent<EventType> event = (AbstractEvent<EventType>) theEvent;<br />switch (event.getType()) {<br /> case Init:<br /> handleInitEvent((InitEvent) event );<br /> case Indicators:<br /> handleIndicators((IndicatorsEvent) event );<br /> break;<br />}<br />
This code looks like a ClassCastException will be thrown whenever we handle the Init event and then fall through to handle Indicators event.

Now I can either add the missing break;, or I can hunt around to confirm that no one is firing the Init event. I decided to hunt around. As it turns out, the Init event is never fired.

I resolved the FindBugs discovery by replacing the dodgy code with:<br />AbstractEvent<EventType> event = (AbstractEvent<EventType>) theEvent;<br />switch (event.getType()) {<br /> case Indicators:<br /> handleIndicators((IndicatorsEvent) event );<br /> break;<br />}<br />
As an added bonus I can remove the handleInitEvent() method. FindBugs excels at spotting neglected code that should be reworked or eliminated.

What dodgy code is hiding in your codebase? Grab yourself a copy of FindBugs and find out.