Sanitising some code

A simple but valuable refactor - converting an interface plus two similar implementations into a final class. This is taken from real code; I wrote this document while refactoring. Here I have an interface that represents an entry in an ARP table.

public interface ArpEntry extends Stringable
{
 MacAddress getMacAddress() throws CheckedIllegalStateException;
 void age();
 boolean dead();
}

Stringable has one method, asString() - I call that instead of toString(), so that I never get java.lang.SomeType@89345aef3 in logs, etc. getMacAddress() may fail, because there are two kinds of ARP entry - some have a MAC address, and some do not (in real implementations, the second kind have a zero MAC address). The ones that have no MAC address are entries to say that a request has been sent. That's why it throws an exception. age() will age the entry by a unit - approximately one second, but as we use this simulation for teaching purposes, we may allow slower students to slow this down. dead() tests to see whether the entry is dead (aged beyond a certain limit). Currently, I have two implementations, both anonymous classes:

public class ArpEntryUtility
{
 public static final int START_TTL=20;

 public static ArpEntry arpEntry(final MacAddress macAddress)
 {
  return new ArpEntry()
  {
   int timeToLive=START_TTL;

   public MacAddress getMacAddress()
   {
    return macAddress;
   }

   public void age()
   {
    timeToLive--;
   }

   public boolean dead()
   {
    return timeToLive<=0;
   }

   public String asString()
   {
    return macAddress.getRawValue()+"; expires in "+timeToLive+" seconds";
   }
  };
 }

 public static ArpEntry incompleteArpEntry()
 {
  return new ArpEntry()
  {
   private int timeToLive=START_TTL;

   public void age()
   {
    timeToLive--;
   }

   public boolean dead()
   {
    return timeToLive<=0;
   }

   public MacAddress getMacAddress() throws CheckedIllegalStateException
   {
    throw new CheckedIllegalStateException();
   }

   public String asString()
   {
    return "incomplete ARP entry - expires in "+timeToLive+" seconds";
   }
  };
 }
}

It's trivial to see duplication here. One approach would be to make the interface a superclass, or to add a superclass that implements the interface, as a base for common code. However, that isn't the only choice. Let's look at a more classic form of code reuse - calling a method. I'll add two methods, getTimeToLive, setTimeToLive, to the interface. These will do no validation, just pass things through. Don't panic, these methods won't live long.

public interface ArpEntry extends Stringable
{
 MacAddress getMacAddress() throws CheckedIllegalStateException;

 void age();
 boolean dead();

 int getTimeToLive();
 void setTimeToLive(int ttl);
}

Now we can implement age() and dead() as static methods in ArpEntryUtility, and call them from the two implementations. Of course, those method calls are still duplication - we can call the static methods directly and remove the methods from the interface. IDEA has a refactor for this whole paragraph - Make Static. It will sort out the callers for you too, changing entry.age() to ArpEntryUtility.age(entry). Now almost the only case-specific code is getMacAddress(). If I change getMacAddress() so that it returns Maybe<MacAddress>, then there's no need for the exception, and hence no need for getMacAddress() to be implemented differently between each implementation.

public interface ArpEntry extends Stringable
{
 Maybe<MacAddress> getMacAddress();
 int getTimeToLive();
 void setTimeToLive(int ttl);
}

Looking at all the use sites, I see that getMacAddress() is only used once, and in that case the exception is caught and converted to a Maybe anyway, so I've just made the use site simpler too, by chance. It looks almost like a struct now, the only case-specific code left is the asString() implementations. I can make that a static method that does different things based on the Maybe<MacAddress>. The two implementations now only differ in how they are constructed. One is passed a MacAddress, the other isn't. Easy to solve, pass a Maybe and now we only have one implementation. One interface, with one implementation. Needless indirection. Let's change the interface to be a final class, and the implementation to just be a constructor call. Finally, we can get rid of the getters and setter, making macAddress a public final field, and timeToLive a public field. That gets rid of some extra needless indirection.

public final class ArpEntry
{
 public final Maybe<MacAddress> macAddress;
 public int timeToLive=20;

 public ArpEntry(Maybe<MacAddress> macAddress)
 {
  this.macAddress=macAddress;
 }
}

public class ArpEntryUtility
{
 public static void age(final ArpEntry arpEntry)
 {
  arpEntry.timeToLive--;
 }

 public static String asString(ArpEntry arpEntry)
 {
  return arpEntry.macAddress.apply(new Lazy<String>()
  {
   public String invoke()
   {
    return "incomplete ARP entry - expires in "+arpEntry.timeToLive+" seconds";
   }
  },new Function<MacAddress,String>()
  {
   public String run(MacAddress macAddress)
   {
    return macAddress.getRawValue()+"; expires in "+arpEntry.timeToLive+" seconds";
   }
  });
 }

 public static boolean dead(ArpEntry entry)
 {
  return entry.timeToLive<=0;
 }
}

You might now decide to restrict the users of ArpEntry so that they have to access everything via the provided static methods. That's fairly simple, just merge the classes and make the fields private. However, I like to keep my 'bags of functions' separate from my instantiable classes. While I was refactoring, one of my automated tests started to fail. It actually took me about two hours to fix the problem. Inadvertently, I had made some of ArpEntry's client code more logical, the code that decides what to do with an outgoing ARP packet from a computer. However, some parent code to that, the code that decides whether to send an outgoing ARP packet) had a logic error, which I'd never noticed before. This refactoring wasn't as straightforward as it could have been, mainly because IDEA doesn't know much about the Maybe type. But overall, I'm pleased I spotted the logic error now, rather than when I'm closer to a release!