Make Concepts Explicit when Fixing Bugs

Dear Junior

To find implicit concepts and make them explicit is a very powerful way to improve code. But, sometimes it is hard to judge beforehand which concept is important enough to make explicit. We try to make good judgements, but sometimes we miss.

But, no reason to despair, we can take Time as our helpful aid. So, instead of everything perfect from start, we continuously perfect it whenever we find a reason. Two of the topmost reasons are either a bug appearing or further development of the code.

Let me for now fokus on the case of when a bug appears. If the code does not behave the way we intended it to do, then there is most often a place in the code that was to convoluted. In that convoluted state, subtle misunderstandings and simple mistakes could hide. The solution is to clarify the code until it is obvious whereof the mistake consists.

Things get clearer by examples, so let me clarify what I mean with a very minimal example, the story about a bug and its fix.

In real life, more or less a decade ago, I worked on a project where we did a batched import of records coming from an external source. The code looked roughly like this following in the class ImportCron.

public class ImportCron {

    private RecordImporter importer;

    private Logger logger;

    void runImport()  {

        List records = importer.listWaitingRecords();

        logger.log(Level.INFO, "Importing records: " + records.size());

        for ( Object rec : records) // imports each of the records in turn

   …

    }

Now, the import was run once a minute, but most of the time there where non records waiting.  Problem was the import log became filled with rows saying "Importing records: 0" so we had to grep out the non-zero lines each time we wanted to look at what was happening.

One of those days I got tired of this, fired up the editor, and wrapped the logging in an if statement to filter out all those zero-import writes.

    void runImport()  {

        List records = importer.listWaitingRecords();

        if (records.size() == 0)

            logger.log(Level.INFO, "Importing records: " + records.size());

        for ( Object rec : records) // imports each of the records in turn

   …

    }

A few days later we released during lunch, which was the best time for our users. The deploy went smooth, apps started up as expected, and soon it was time for the first import "tick".

Within a minute, import was run the first time, and the log line read:

<timestamp> Importing records: 0

While standing confused, a minute went by, and the log line read:

<timestamp> Importing records: 0

Another minute went by … and no log line.

While firing up my editor to check the code, another minute went by and a third log line appeared:

<timestamp> Importing records: 0

A colleague of mine looking in the database calmly reported "We just imported three records".

It only took me a split of a second to realise my mistake once I had the code in front of me.

    void runImport()  {

        List records = importer.listWaitingRecords();

        if (records.size() == 0)

            logger.log(Level.INFO, "Importing records: " + records.size());

        for ( Object rec : records) // imports each of the records in turn

    }

Obviously my spine reflex for coding "check for zero" made me write the boolean expression the wrong way around. By the way, did you capture that mistake at first glance in the code on my first description? Confirmation bias is a nasty thing.

Now, I must say this kind of bugs are pretty uncommon, bugs that are typos or simply mispunching the keys. Most bugs in my opinion are rather that pieces of code subtly misunderstand each other.

Well there are at least two ways of fixing this code. The obvious, and fastest would be to quickly change "==" to "!=". However, humbled by the mistake I had done in the first place, I realised that that kind of hasty coding was what got me in the trouble in the first place.

One of my coding mantras since long have been "Code should mean something, not just do something". So, a better way out would be to make the code more meaning-ful. From Eric Evans I learned the phrase "Make implicit concepts explicit", which says the same thing in this context, but gives a better guiding direction forward.

I took to the challenge of fixing the bug by finding what implicit concepts had been missed, and making them explicit until it was blatantly obvious that the code was wrong. An added benefit would be to be able to make a test proving the code was wrong.

Extracting the boolean condition to a method of its own would force me to spell out the meaning of that piece of code.

    void runImport()  {

        List records = importer.listWaitingRecords();

        if (containsRecords(records))

            logger.log(Level.INFO, "Importing records: " + records.size());

        for ( Object rec : records) // imports each of the records in turn

    }

    static boolean containsRecords(List records) {

        return records.size() == 0;

    }

Granted, this is more code than I started with - but I think the code is more "to the point" (phrased inspired by Rickard Öberg, another of the great programmers).

At least this made me able to write a test

    @Test

    public void shouldConsiderEmptyListNotContainingRecords() {

       Assert.assertFalse(ImportCron.containsRecords(Collections.EMPTY_LIST));

    }

Well, at least this is what the test would look today - at the time JUnit looked slightly different.

Anyway, now I have a failing test. Claiming "the empty list does not contain records" is simply false - as proven by the test. We safely update the code to fix the bug.

    static boolean containsRecords(List records) {

        return records.size() != 0;

    }

Upon which the test switched to green as expected.

During this "refactor -> put under test -> fix" something interesting has happened. The concept "import list contains records" that hitherto had been implicitly represented by the technical "records.size() != 0" has now been made explicit and given a name "containsRecords".

It might be claimed that the main benefit in this example was derived from the drive to put code under test before making a change. Undeniably, that is a point, but I think it only tells half the tale. 

Simply putting things under test can be done in a myriad of ways, and I have seen several very technology-based efforts. I do not think those efforts have paid off handsomely. Mostly the code get cut along technical lines to push in tests in the gaps. But, the code does not get more comprehensible over time.

Focusing on "make implicit concepts explicit" have been a more productive course for me when working with code.

Yours

   Dan

ps My team-mates did not force me to skip lunch to fix the bug. We went out together, and when we came back satisfied and rested, I sat down and fixed the code. We released the fix during lunch-time the day thereafter.

Make Implicit Concepts Explicit in Code

Dear Junior

In our letters on software development we have touched upon the idea several times, but I think it might be worth spelling it out - the idea to make implicit concepts explicit; something I see as one central message of Domain Driven Design applied to coding.

Let me pick that phrase apart for a moment. When coding we have a lot of concepts in mind, e g when writing code that handles people I might have a class namned Person. In this case the concept of person has an explicit representation in the code - i e an explicit concept.

A person might have a birth date, represented in code as a data-field Date dayofbirth in the Person class; another example of an explicit concept.

We might have business logic restrictions that are based on the age of the person, e g you have to be at least fifteen years to access some content. So there will be code calculating this.

    void contentRequest() {
        ... 
        boolean access = ((new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear) >= 15;

Again we have a concept represented in code, this time age. However, this time the representation is not explicit - there is nothing in the code saying "age". Age is an implicit concept. 

There is nothing strange with having implicit concepts. In the short code snippet we have several implicit concepts: current point of time, point of time of customer's birth, and age-limit for content are just three obvious. This is not a problem. The programmer reading the code will intuitively re-construct the relevant concepts.

However, the design guideline from Domain Driven Design advice us to keep an eye open for important concepts - and if we find the represented implicitly, then make that implicit concept explicit. 

Let us look at the code snippet again.

        boolean access = ((new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear) >= 15;

Of the implicit concepts dwelling in this like, which of them seem important enough to make explicit? 

The concept of "current point of time"? Probably not.

The concept of "milliseconds since birth"? Nah, not that either.

The concept of "milliseconds since birth expressed as years, rounded downwards"? Hey! I know this! We already have a word for it - we call it "age"! And we talk about it all the time!

Well, that sounds important enough. Let us start with giving that value a name. 

    void contentRequest() {
        ... 

        long age = (new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear;

        boolean access = age >= 15;

By the way: this is where I really like the IntelliJ shortcuts "cmd-w" to widen the selection until the expression is selected, then "cmd-alt-v" for the refactoring to extract the selection as a variable - naming it "age" in the pop-up. It literally takes less than 10 seconds.

Now, we actually have made the implicit concept age into an explicit concept. Mission completed.

Well, not completely. We need to find the concept a good home where it can lead a good life. Right now it is stranded in the middle of some access computation. At least we can give it a method of its own.

Applying another of my favourite refactoring "Replace Temp with Query" yields this code.

    void contentRequest() {
        ... 
        boolean access = customerAge(customer) >= 15;

    private long customerAge(Person customer) {

        return (new Date().getTime() - customer.dayofbirth.getTime()) / msPerYear;

    }

Better, but can be improved to make the concept clearer. If we talk about the "age of the customer" it will vary from time to time, and it might cause confusion: the age when the customer requested access to the content, the age when content was first accessed, when it was last accessed, the age now? Better to clarify.

Of course we could rename the method to "customerAgeNow". However, I do not feel comfortable having my concepts depend implicitly on external things like the system clock. I prefer to make those dependencies explicit. So we make the time-in-question a parameter. 

    void contentRequest() {

        ... 

        boolean access = customerAgeAt(customer, new Date()) >= 15;

    private long customerAgeAt(Person customer, Date timepoint) {

        return (timepoint.getTime() - customer.dayofbirth.getTime()) / msPerYear;

    }

 

This design also improves testability drastically. The tests can now use explicit test-data, and need not rely on checking, or changing, the system clock.

Kudos once again to the lovely refactoring support of modern IDEs. Another less-than-10-seconds-refactoring: two cmd-W to select "new Date()", cmd-alt-P to extract as parameter, change name to "timepoint" in pop-up. Enter. Done.

By now it is pretty obvious that the method "customerAgeAt" suffers from feature envy. The most relevant data it operates upon is the Customer, but it resides somewhere else. We should consider moving it.

Should we move it, the Customer class would get a method "ageAt", taking the liberty to rename it slightly. That method certainly makes sense in this context, but does it so in other contexts? As we do  not have the rest of the codebase at hand, we will just have to pretend that "ageAt" makes sense in other context - and might actually be useful there as well.

This is really one of my favourite refactoring: move method. Again extremely smooth thanks to modern IDEs. Literally two keystrokes (F6, Enter - in IntelliJ) for the move, and a few for renaming. 

    void contentRequest() {

        // ...

        boolean access = customer.ageAt(new Date()) >= 15;

class Person {
    Date dayofbirth;
    // ...
    long ageAt(Date timepoint) {
        return (timepoint.getTime() - dayofbirth.getTime()) / TimeUtil.msPerYear;
    }
}

You might not believe me, but I remember the days of yore when you actually had to do all the involved steps manually; cut-n-paste the method body and header, change the list of parameters, update the code to use the fields of "this" instead of the argument, change all client calls (which might be several).

Now, we have finally ended up with a version I feel comfortable with. The concept "person" has now an conceptual attribute "age at" which has an explicit representation in code. In code we have "enhanced" our language of what we can talk about directly. So, when we discuss the age of a customer with business people, it is likely that we consistently mean the same thing - even if we for sure still can misunderstand each other.

As a side effect the code checking the access has become a lot clearer.

        boolean access = customer.ageAt(new Date()) >= 15;

This is a line of code that can be shown some person from the business side and explained by reading it out "access is granted if the customer's age at 'now' is at least fifteen years". Given that support, they can see by themselves - something that really builds trust. 

The time invested was not huge. Coding-wise all refactorings took less then a minute together. The time to slowly realise that "age" is an important concept in this particular domain is not counted. However, that insight will have to dawn on the programmer sooner or later, and when that happens, that extra minute is well invested.

To make concepts explicit does not only apply to code, it can be fruitfully applied to other areas as well, e g capturing and writing requirements/specifications. 

As we all stand on shoulders of gigants, I also must give credit where credit is due. The phrase "Make Implicit Concepts Explicit" I have picked up from Eric Evans, the thought leader of Domain Driven Design. 

In conclusion: to make implicit concepts explicit help us to over time close the gap between the code and the understanding of the business domain. It is not uncommon to in the process find bugs or crucial misunderstandings, that then can be adressed in a proactive manner instead of popping up as nasty surprises later on. 

Yours 

   Dan

Getting Started with JUnit and TDD in Scala

Dear Junior

As I like the flow of test driven development, it was natural to me to try TDD when starting using Scala. As Scala runs on the JVM and interoperates well with Java it should be easy to just use JUnit. However, it was not totally obvious how to do it, and looking around the net I found no "start from scratch, this is how to do it"-tutorial. Anyway, this is how I did it, starting from scratch.


A First Test - without JUnit

Let's start with writing a test for the now-classical Money-example. Using my IDE at hand (IntelliJ), I created a file 'MoneyTest.scala' and wrote the test checking that one swedish krona is one swedish krona.

class MoneyTest {

  def should_equal_same_amount_same_currency = {

    new Money("SEK", 1) == new Money("SEK", 1)

  }

}

Let us pick this apart. This is a class (MoneyTest) that can be used to create instances of that class. Such an instance ("a MoneyTest") is in other words "an object that can test properties of Money". One of the properties it can test is that Money "should equal same amount same currency". Testing the property is done by running the method.


We have not yet got JUnit in the loop, but this is the anatomy of a unit test in place.


Get it to Compile


To get the test through the compiler we need a Money class as well, which will reside in Money.scala.

class Money(currency : String, amount : Int) {}

Yepp, that's it - class declaration and constructor rolled together.

Now we are in shape having our first unit test and enough implementation to satisfy the compiler. Time to run the test. If we want to we can run it on the interactive read-eval-print-loop (REPL).

scala&gt; new MoneyTest().should_equal_same_amount_same_currency  

res0: Boolean = false

Well, this is actually a failing unit test, giving us the permission to hack away. 


Taking the Red Test to JUnit

However, I have come to be used to JUnit - I like the assert methods, I like how it integrates with my IDE, I like how it integrates with build servers. So, I would like to pimp up this pretty naked unit test to make use of JUnit.

First step was to add JUnit to the Scala project. In IntelliJ I created a new "External Library" named "junit" to which I added junit-4.4.jar - I happened to find it in ~/.m2/repository/junit/junit/4.4/junit-4.4.jar. Then I added the new external library to the project.


Next step is the code. As JUnit is now on the classpath, I can do the imports, annotations, and use the methods I am used to (albeit the import will be Scala-scented). 

import org.junit.Test

import org.junit.Assert._

class MoneyTest {

  @Test

  def should_equal_same_amount_same_currency = {

    assertTrue(new Money("SEK", 1) == new Money("SEK", 1))

  }

}

Looks familiar, doesn't it? Now I can run it from within IntelliJ like any JUnit-test. Running it gives another familiar message:

java.lang.AssertionError: 

at org.junit.Assert.fail(Assert.java:74)

at org.junit.Assert.assertTrue(Assert.java:37)

at org.junit.Assert.assertTrue(Assert.java:46)

at MoneyTest.should_equal_same_amount_same_currency(MoneyTest.scala:10)

...

Well, using assertTrue never gave helpful error messages. Let's do better.


Using JUnit


Let us change the assert and use assertEquals instead. This works fine as the mother-class in Scala (named AnyRef) is nothing but "java.lang.Object" so there will be an equals method around.


To be precise, there is actually a class above "AnyRef" named "Any" which include the immutable value-types in Scala that parallel the Java primitives. But that distinction does not matter here.


Changing the code to use assertEquals we get:

  @Test

  def should_equal_same_amount_same_currency = {

    assertEquals(new Money("SEK", 1), new Money("SEK", 1))

  }

 This lands me with the error message:

java.lang.AssertionError: expected:<Money@348bdcd2> but was:<Money@4a4e79f1>

at ...

at MoneyTest.should_equal_same_amount_same_currency(MoneyTest.scala:10)

OK, two Money instances with different hashcodes are obviously not considered equal, even though their data are the same. This might be what we want for entities or domain events, but not for value objects such as Money.

However  the error message looks kind of obscure due to the default toString. Let us add a custom toString to get readable messages in the future.

class Money(currency : String, amount : Int) {

  override def toString() : String = currency + " " + amount

}

Running the tests again gives:

java.lang.AssertionError: expected: Money<SEK 1> but was: Money<SEK 1>

at ...

This is definitely more readable. Arguably it would be more confusing had we not remembered the last error message: they are different instances. So, the equality test that Scala gives us from the Scala top class AnyRef (which is the same as java.lang.Object on the JVM) does not cut it for value objects - exactly as in Java.


Fixing Equality

What to do? Of course we could get into Money and add an override def equals returning true. Then we could add a testcase to provoke a failing equals, forcing us to elaborate our equals method etc. However, in Scala there is a simpler solution at hand - declare Money as a "case class".

case class Money(currency : String, amount : Int) {

  override def toString() : String = currency + " " + amount

}

Let us not get into the details, but apart from giving us well-behaving equals and hashcode, it actually makes sense for value objects to be case classes.

Green Test, Refactoring, and Ready to Start over again


The good part is that this change take us home to a green bar. There is not much refactoring to do, so now we have gone a full TDD cycle using Scala and JUnit. Time for next round. 


Now we have gotten started

Yours

   Dan 


ps Once getting started, you might want to try out some actors in scala.

Demeter Saves Mocking Fairies

Dear Junior

In a Twitter post Damian Guy recently expressed his frustration with some hard-to-unit-test code as “”Every time a mock returns a mock, a fairy dies”. (Twitter @damianguy 2009 Oct 19)

I immediately fell in love with the quote and how it “backwards” refers to a guideline on how to design (or not design) your code for testability. What has struck me recently is that this design guideline is actually nothing but the battle-proven Law of Demeter (“Don’t Talk to Strangers”).

Let me give a rather sketchy example of how you could get a report of the shipping weight of an order composed of order items.

OrderService.reportTotalWeight(order)

   orderitemlist= order.items()

   weight = 0

   for(item in orderitemlist)

      weight.add(item.weight);

   return weight;

So, what is the problem? Well, imagine the unit test for the report functionality in OrderService. To properly unit test that functionality we need to rig a “test bench” with mocked versions of the “surrounding” objects. If we do not use mocks, we will actually also test the functionality in the classes of the surrounding objects as well, and then we are not really “unit-testing” any longer – more like “micro-integration-testing”.

So a setup would need to pass in a mocked order. But then comes the call “order.items()” upon which the mock need to pass back a list of objects. Further on, these objects will have their “weight” method called, so they need to be mocks as well …

OrderServiceTest.shouldSumOrderItemWeightsForOrderWeightReport()

   // Given

   order = new Mock()

   item1 = new Mock().expect(weight).return(4711)

   item2 = new Mock().expect(weight).return(42)

   order.expect(items).return([item1,item2])

   // When

   orderservice.reportTotalWeight(order)

   // Then

   …

Messy, messy, messy - and we have a mock that returns mocks. Not good for the fairy population.

The underlying design problem is that the OrderService “reaches beyond” the order object to work on the order item objects. By doing this, it "extends its contact surface" to the rest of the application – and makes it hard to write a simple and complete unit test.

Well, the Law of Demeter is a 20 years old design rule in object orientation that says that an object should only talk to its immediate neighbours. The definition found in e g Wikipedia is that the allowed objects are:

• The object itself
• Method parameters
• Any objects created/instantiated within the method
• The object’s direct component objects

In many OO-supporting languages build upon C syntax, this can be viewed as “just one dot” – this is not precise, but gives the right intuition about the rule.

And here we see that the order items do not fit into that list from the point of view of the order service.

Redesigning to follow Law of Demeter would limit OrderService to reach to order, but not to order items. This requires a new method in Order: “weight of all items”.

OrderService.reportTotalWeight(order)

   weight = order.weighOfAllItems()

   return weight

This piece of functionality is obviously trivial to unit test (in case you insist), and will only require you to mock one object, the order.

What has happened is of course that the complexity have moved elsewhere – into the order and its “weight of all items” method.

Order.weightOfAllItems

   orderitemlist= this.items()

   weight = 0

   for(item in orderitemlist)

      weight.add(item.weight);

   return weight

It is the same code as earlier – with “order” changed to “this” (and some obvious refactoring simplifications). Now unit testing of this only requires one level of mocks, as the code only talks to “its immediate neighbours”.

OrderTest.shouldSumOrderItemWeights ()

   // Given

   item1 = new Mock().expect(weight).return(4711)

   item2 = new Mock().expect(weight).return(42)

   // When

   order.add(item1).add(item2)

   // Then

And no fairies where killed in the testing of this code.

What I find beautiful is that these two ways lead towards the same goal. Law of Demeter was an object orientation theory invention to address coupling and coherence of designs. The save-the-fairies quote came from a pragmatic culture of wanting to produce high-quality code. I just love that they actually mean the same.

Well, when the Law of Demeter was coined, TDD was not around, “mock” meant something else, and fairies was not to be hip for a decade or two. So, finally I must praise Damian for formulating such a modern phrasing of a piece of ancient wisdom.

Yours

Dan

ps To be able to easily write unit tests without neither killing fairies nor "mocking yourself to death" is of course crucial for TDD to be fun.

pps Realising that redesigning your code for testability is of cause an effect of wanting to test the behaviour of the code, not the code per se.

Psychology of Tests, Testing and TDD

Dear Junior

There have been lots of talk about the benefits of test driven development, but there is one element that I think is interesting that I not have seen a reference to - the psychological process.

Pretend that we dive into the head of a programmer (e g me) and see what happens during testing. Let us firest have a look at "classical" post-fact tests and then at the TDD way of writing the test before the code.

If I write some functional code and consider myself finished I will without doubt feel immensely proud of myself - in other words my ego is on a high note. Then I sit down to test it, and let's see what can happen: either I find bugs and flaws, or I don't. If I do not find any bugs, I do not get any extra credits in my own books - I have been exactly as good as I thought I was (but I might feel that I have wasted some time). If I do find bugs I get a slap in the face - I have not been as good as I though I was (and have committed hubris, which is a severe sin on the list of things considered as sins in Sweden). So, even if testing will improve the general level of the result, the activity of testing (per se) will never give an upside, can at best have a zero outcome, and at worse a big minus. Depressing.

So, how do I treat activities that give me no upside, but possible downsides? Naturally, I tend to avoid them, and over time minimize them.

So, I tend to shy away from testing and writing tests; even though I am firmly convinced of their necessity. Depressing.

Driving the development through tests breaks that negative feedback loop. If I write the test before the code, naturally it will fail: I have no other expectation, and the red bar is not a failure. Later on, I write some code, which make the test go green, and I have actually finished on a high note. Great!

Well, actually I have not finished as any TDD-savvy would quickly point out. I have not refactored away duplication and other code smells yet. But doing so will satisfy another part of my ego, the code aesthetics lover that resides inside me. So, satisfying that part of my ego will end the job on a yet higher not.

OK, to be brutally honest: are there no part of TDDs "red-green-refactor" that have the same "can only go wrong" drawback as traditional test-last? Well, there actually are: the refactoring. At best it says: you did not mess up, at worse: you think you where so smart but you failed. So, how come I do not tend to neglect refactoring?

Well, I have already given the answer: I love refactoring, if nothing else for the pure love of nicely crafted code. But also, the refactorings are not only the last cleanup of one part of the job. They are most often also the stepping-stone from which I launch into the next part of functionality to write - so I get immediate feedback from the refactoring as well.

I think writing tests in general, and unit tests in particular are really important. Considering how many that agree, and how seldom it is practiced, we must look for explanations: ignorance, arrogance or malice do not suffice. To see how we experience "writing tests" has helped me to shed some light over the mystery.

Whatever, I think one of the drives for me to do TDD is that is make all parts of the development cycle to be fun.

Yours

Dan

Don't Test Your Code

Dear Junior

From time to time I get asked how to test a specific piece of code (EJB2:s or Action classes being nasty examples). Most of the times I spontaneously do not want to answer the question at all, because I think it lead us in the wrong direction.

I do not want to test my code; I want to test the behaviour of my code. The distinction might seem like hair-splitting, but I think it makes an important point. For example, if I have some order payment service I want to check that the payment information is well-balanced, that a payment request is sent to the bank, and a lot of other stuff. If we "test the code" we want to ensure all of these things as they are written.

However, if we focus on testing the behaviour we want to make sure that one idea ("generated book-keeping transaction is well-balanced") is correctly realised by the code. This approach enables us to focus on one aspect of the behaviour at a time and ask: How can I make it possible to test this behaviour. And the natural answer is to restructure to obtain testability.

So, phrasing the test-question "how do I test my code" leads us to build really elaborate testing frameworks to feed test data into the database, and then running our components in a production-like environment (e g Cactus tests). On the other hand, phrasing the question "how do I test the behaviour of my code" leads us to refactor until we have separated concerns to such a degree that each idea is possible to test in isolation.

So, don't test your code; test its behaviour.

Yours

Dan

ps One thing you can do if you redesign for testability, is that you can refactor your code to require less cumbersome and awkward mocking.