Refactoring to Law of Demeter

The Law of Demeter (paraphrased as “Tell, Don’t Ask”) is the developer equivalent of an early morning stretch. If you wake up stiff and sore, running through a few gentle stretches can tease out some of the muscles that have been under too much load, and make you feel limber again.

Recently, I’ve been a bit of a one-trick pony when it comes to writing code – I just say “use the Law of Demeter” and expect people to know where I’m going. I think that’s a bit harsh (especially on my colleagues), so I’d like to mumble a few words about how it can work in practice.

I’ve also had pub-code conversations around this, and for people who use Demeter on a regular basis, the question is where do you stop? I think I’ll show my answer to this, but first:

The scenario:
We have various components in our system, and we want to generate a health check page that can help production support quickly diagnose high severity failures. All we need to do is generate a web page for those components…

The setup:
We have an old-school MVC web app – We use a templated view, our view Model is a hierarchy name/value pair structure called “ViewData”, and we’re using Typed Constructor Dependency Injection.

The first draft of code looks like this

class DatabaseStatus {
  boolean allOk = true;
  public DatabaseStatus( HibernateDatabase database, Session session, DeploymentInfo info ) {...}
  public void renderTo( ViewData data ){
    List statusItems = new List();
    Properties props = addStatusForPropertyFile( statusItems );
    addStatusForDatabaseUsername( database, statusItems )
    addStatusForDatabasePassword( database, statusItems )
    addStatusForDatabaseUrl( database, statusItems )
    addStatusForDatabaseDriver( database, statusItems )

    allOk = true;
    for( StatusItem item : statusItems ){
       ViewData statusView = data.addItemToList( "status" );
       statusView.addEntry( "name", status.getName() );
       statusView.addEntry( "description", status.getDescription() );
       statusView.addEntry( "status", "ok".equalsIgnoreCase(status.getStatus())? "ok" : "issue" );
       allOk = allOk && status.getStatus();
    }
  }
}

So, it’s ok. It has some tests around it as well, but there are some obvious points of improvement.

    The addStatusToX methods smack of duplication
    This class has feature envy of StatusItem
    There is a magic value for getStatus() – which maybe used to turn on some special styling
    It’s difficult to unit test, since all the specific dependencies have to be setup

After a day of refactoring, we ended up with the following:

class DatabaseStatus {
  public DatabaseStatus( Health[] allHealth ) {...}

  public void renderTo( final ViewData viewData ){
    final MutableBoolean allOk = true;
    HealthReport report = new HealthReport() {
      boolean allOk = true;
      public void addHealthStatus( String name, String description, HealthStatus status ){
         ViewData statusView = viewData.addItemToList( "status" );
         statusView.addEntry( "name", name );
         statusView.addEntry( "description", description);
         statusView.addEntry( "status", status.isHealthy()? "ok" : "issue" );
         allOk = allOk && status.isHealthy();
      }
    }
    for( Health health : allHealth ){
       health.addToHealthReport( report );
    }
  }
}

interface Health {
  void addToHealthReport( HealthReport report );
}

interface HealthReport {
  void addHealthStatus( String name, String description, HealthStatus status );
}

enum HealthStatus {
  Healthy,
  Unhealthy

  public boolean isHealthy(){
    this == Healthy
  }
  static HealthStatus trueIsHealthy( Boolean value ) {
    return value.booleanValue()?Healthy:Unhealthy;
  } 
}

So by following the Law of Demeter:

    The DatabaseStatus class gets told what health is relevant.
    Each health is told what report object to write into
    The inline HealthReport encapsulates the mapping to the View, and tells it what to render

The benefits:

    It’s MUCH easier to unit test – the full range of health behaviour can be simulated now
    Each Health has to only worry about Health, not how that will be represented
    Adding more entries to the HealthReport is straightforward, and done in the object container

The interesting:

    The name of “HealthReport” makes things clearer, and allows view abstraction (e.g. using a JmxHealthReport)
    This seems a mix of object-oriented and functional code, albeit with some mutable state
    Using enums (e.g. HealthStatus) rather than primitives allows more refactoring options, since it is straightforward to push behaviour (the ‘tell’) into the enum

The introduction of HealthReport helps to crystalise where the “action” is. I think there may be a general case here – that in practice, modelling the interaction between objects will often drive out a new object encapsulating that interaction.

    When a Boy kisses a Girl – the kiss itself is important
    When money is transferred from a source account to destination account – the payment itself is important
    When a car and a wall collide – the collision is important
Advertisements

One Response to Refactoring to Law of Demeter

  1. […] Again I’ve simplified the API to show the idea of delegating responsibility for how we render the control to the EditMode. Nick has written more about this idea in a post about refactoring to the law of demeter. […]

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: