Refactoring

Perhaps there are already enough refactoring examples, but if not then I present yet another. I was writing some code investigating a potential project here at Venture, and since it was just spike, code, I just hammered it out. Here's run() from a Runnable:

    public void run() {
        Map fileDetails = new Hashtable();
        try {
            BufferedReader reader = new BufferedReader(new FileReader(MainFrame.WATCHED_DATA));
            String directoryName = reader.readLine();
            File fileData = new File(directoryName, ".vobas");
            while (directoryName != null) {
                if (!fileData.exists()) {
                    fileData.createNewFile();
                } else {
                    ObjectInputStream fileDetailsReader = new ObjectInputStream(new FileInputStream(fileData));
                    FileDetail fileDetail = (FileDetail) fileDetailsReader.readObject();
                    while (fileDetail != null) {
                        fileDetails.put(fileDetail.getName(), fileDetail);
                        try {
                            fileDetail = (FileDetail) fileDetailsReader.readObject();
                        } catch (EOFException e) {
                            break;
                        }
                    }
                }
                File[] files = new File(directoryName).listFiles();
                for (File file : files) {
                    FileDetail fileDetail = fileDetails.get(file.getName());
                    if (fileDetail == null) {
                        ScpTo.send(directoryName + File.separatorChar + file.getName());
                        fileDetails.put(file.getName(), new FileDetail(new Date(), file.getName()));
                    } else if (file.lastModified() > fileDetail.getModificationDate().getTime()) {
                        ScpTo.send(directoryName + File.separatorChar + file.getName());
                        fileDetails.remove(file.getName());
                        fileDetails.put(file.getName(), new FileDetail(new Date(), file.getName()));
                    }
                }
                ObjectOutput objectOutput = new ObjectOutputStream(new FileOutputStream(new File(directoryName, ".vobas")));
                for (FileDetail fileDetail : fileDetails.values()) {
                    objectOutput.writeObject(fileDetail);
                }
                objectOutput.close();
                directoryName = reader.readLine();
            }
            reader.close();
        } catch (FileNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (ClassNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

If you're anything like me, you'll need a few minutes to compose yourself after reading this. If you read that code and didn't immediately feel a little ill, I strongly suggest you pick up Clean Code by Robert Martin. Code like this should make you at least a little queezy.

Abstractness In Methods

I learned from Clean Code that all methods should have the same level of abstractness. Public highlevel methods should do high level things. They should be composed of medium abstract methods that are themselves composed of highly detailed methods that do lowlevel things.

For example this code

ObjectOutput objectOutput = new ObjectOutputStream(new FileOutputStream(new File(directoryName, ".vobas")));
for (FileDetail fileDetail : fileDetails.values()) {
  objectOutput.writeObject(fileDetail);
}
objectOutput.close();

writes FileDetails to a file. It can be extracted into a self describing method

writeFileDetails(fileDetails, directoryName);
...

where writeFileDetails looks like

private ObjectOutput writeFileDetails(Map fileDetails, String directoryName)
    throws IOException, FileNotFoundException {
  ObjectOutput objectOutput = new ObjectOutputStream(new FileOutputStream(new File(directoryName, ".vobas")));
  for (FileDetail fileDetail : fileDetails.values()) {
    objectOutput.writeObject(fileDetail);
  }
  objectOutput.close();
  return objectOutput;
}

The section just above that starts with File[] files = new File(directoryName).listFiles(); reads the files from the directory, checks to see if each file needs passed to ScpTo, then updates the FileDetail map. With judicious inlining, renaming, and extract method the for loop can be refactored to

for (File file : eachFileIn(directoryName)) {
  if (fileModified(file, fileDetails.get(file.getName()))) {
    ScpTo.send(directoryName + File.separatorChar + file.getName());
    fileDetails.put(file.getName(), new FileDetail(new Date(), file.getName()));
  }
}

and then replaced with a self describing method

sendModifiedFiles(fileDetails, directoryName);

The fileDetails Map is starting to look pretty interesting to me. It seems this naked collection should be made a first class object. I'll repress this urge for now, but I wanted to mention it during the same part of the process that it occurred to me.

My run method is starting to fit on one page. Always a good sign. Arguments over method length can be as heated and never ending as brace wars, but while we can't seem to agree on one or two line methods, most can agree they should fit on one page in your editor.

The if/else clause is about reading from the file detail file and loading up the fileDetails Map. Let's extract that method so

if (!fileData.exists()) {
  fileData.createNewFile();
} else {
  ObjectInputStream fileDetailsReader = new ObjectInputStream(new FileInputStream(fileData));
  FileDetail fileDetail = (FileDetail) fileDetailsReader.readObject();
  while (fileDetail != null) {
    fileDetails.put(fileDetail.getName(), fileDetail);
    try {
      fileDetail = (FileDetail) fileDetailsReader.readObject();
    } catch (EOFException e) {
      break;
    }
  }
}

becomes

readFileDetails(fileDetails, fileData);

Now we can extract the try clause into a very abstract method.

try {
  BufferedReader reader = new BufferedReader(new FileReader(MainFrame.WATCHED_DATA));
  String directoryName = reader.readLine();
  File fileData = new File(directoryName, ".vobas");
  while (directoryName != null) {
    readFileDetails(fileDetails, fileData);
    sendModifiedFiles(fileDetails, directoryName);
    writeFileDetails(fileDetails, directoryName);
    directoryName = reader.readLine();
  }
  reader.close();
}

becomes

try {
  sendModifiedFiles(fileDetails);
}

The two page try clause has been reduced to a single method call that explains succinctly what the method is trying to do. By breaking the long try clause into small chunks and extracting self describing methods, I've converted a fairly incomprehensible chunk of code into a self documenting class. I'll save wrapping the file details Map in a class and moving most of the methods to it since it seems to be the one doing all the work for another entry. Until then, here is the resulting class:

public class VobasBackupService implements Runnable {

    public void run() {
        Map fileDetails = new Hashtable();
        try {
            sendModifiedFiles(fileDetails);
        } catch (FileNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        } catch (ClassNotFoundException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
    }

    private void sendModifiedFiles(Map fileDetails) throws FileNotFoundException, IOException,
            ClassNotFoundException {
        BufferedReader reader = new BufferedReader(new FileReader(MainFrame.WATCHED_DATA));
        String directoryName = reader.readLine();
        File fileData = new File(directoryName, ".vobas");
        while (directoryName != null) {
            readFileDetails(fileDetails, fileData);
            sendModifiedFiles(fileDetails, directoryName);
            writeFileDetails(fileDetails, directoryName);
            directoryName = reader.readLine();
        }
        reader.close();
    }

    private void readFileDetails(Map fileDetails, File fileData) throws IOException,
            FileNotFoundException, ClassNotFoundException {
        if (!fileData.exists()) {
            fileData.createNewFile();
        } else {
            ObjectInputStream fileDetailsReader = new ObjectInputStream(new FileInputStream(fileData));
            FileDetail fileDetail = (FileDetail) fileDetailsReader.readObject();
            while (fileDetail != null) {
                fileDetails.put(fileDetail.getName(), fileDetail);
                try {
                    fileDetail = (FileDetail) fileDetailsReader.readObject();
                } catch (EOFException e) {
                    break;
                }
            }
        }
    }

    private void sendModifiedFiles(Map fileDetails, String directoryName) {
        for (File file : eachFileIn(directoryName)) {
            if (fileModified(file, fileDetails.get(file.getName()))) {
                ScpTo.send(directoryName + File.separatorChar + file.getName());
                fileDetails.put(file.getName(), new FileDetail(new Date(), file.getName()));
            }
        }
    }

    private boolean fileModified(File file, FileDetail fileDetail) {
        return fileDetail == null || file.lastModified() > fileDetail.getModificationDate().getTime();
    }

    private File[] eachFileIn(String directoryName) {
        return new File(directoryName).listFiles();
    }

    private ObjectOutput writeFileDetails(Map fileDetails, String directoryName)
            throws IOException, FileNotFoundException {
        ObjectOutput objectOutput = new ObjectOutputStream(new FileOutputStream(new File(directoryName, ".vobas")));
        for (FileDetail fileDetail : fileDetails.values()) {
            objectOutput.writeObject(fileDetail);
        }
        objectOutput.close();
        return objectOutput;
    }
}

If you build it they will come

Bill posted a comment to my last blog about seeing long methods and classes with too many responsibilities rather than the other way around. I posted a reply, but I though the reply was worth a short blog.

I like to keep a close eye on primitive instance variables, including Strings. When they start to look at all interesting, I make a class out of them. By interesting I mean that code starts to care about more than just the value. Perhaps decisions are being made, or they are used to influence other instance variables. Perhaps a groups of instance variables are palling around together. Those are all signs of a missing object. Even if it starts out as just a data object with a constructor and getters. Objects are like the Field of Dreams, if you build them, behavior will come.

How to Write Great OO Day One

There's no shortcut to experience. Writing good object oriented code takes experience, but here are three practices to help you get off on the right foot day one with even the grumpiest of gray beards:

1. Write all your code using Test Driven Development(TDD)


2. Follow the Rules of Simplicity


3. Tell Don't Ask

Write All Your Code Using TDD

Code written test first and code written test last is very very different code. Code written test first is loosely coupled and highly cohesive. Code written test last often breaks encapsulation when some property or private method needs to be exposed to the test because the class wasn't designed to be tested. If you write your code test first, your dependencies will be better and your code will be loosely coupled and highly cohesive. More on this later.

Follow the Rules of Simplicity

Code is simple when it:

1. Runs all the tests


2. Contains no duplication


3. Expresses all the intent


4. Utilizes the fewest classes and methods

It's important to notice that I've used an ordered list. The order is important. A single GodClass with a single main() method is not simple. It may run all the tests, but in any program more complex than "Hello, world!" it certainly will contain duplication and not express all the intent.

My struggle with the Rules of Simplicity focused around the If Bug. I didn't understand how following the rules of simplicity would head off someone writing if heavy code. One could argue, and I tried, that if heavy code does not express intent. But when you read code like

if (mobile.getType() == MobileTypes.STANDARD) {
  alert();
}

It's pretty darn easy to see the intent. In the context of whichever method this is in, if the mobile is of type STANDARD then alert. How much more intent do you need?

Then I had a little epiphany. If there's code like that, then certainly in other places in the code there's more. There's probably code like:

if (mobile.getType() == MobileTypes.GAS) {
  registerGasReading();
}

and

if (mobile.getType() == MobileTypes.TEXT) {
  sendTextMessage();
}

and

if (mobile.getType() == MobileTypes.LOCATION) {
  notifyLocation();
}

Do you see it? I sure do. Violations of rule 2. Many many violations of rule 2. And the worst kind of violations of rule 2. Duplication in many different pieces of the code. Duplication that is going to be very very hard to find. So to help prevent this, I've included

Tell Don't Ask

Tell Don't Ask simply means don't ask an object about it's state and then do something. Tell the object to do something. This means all those if examples become:

mobile.alert();

and

mobile.registerGasReading();

and

mobile.sendTextMessage();

and

mobile.notifyLocation();

Now suppose that there were some of those if clauses splattered throughout the code that had duplicate implementations. In the if heavy version, they would be very hard to find, but in the tell don't ask version, all the implementations are in Mobile. All in one place and ready for sniffing out and eradicating.


Listening to your tests will also help you keep your code simple.

public interface Alarm {
  void alert(Mobile mobile);
}

public class Siren implements Alarm {
  public void alert(Mobile mobile) {
    if (mobile.getType == MobileTypes.STANDARD) {
      soundSiren();
    }
  }
}

public class TestSiren extends TestCase {
  public void test_alert() {
    LocationMobile mobile = new LocationMobile();
    Siren siren = new Siren();
    siren.alert(mobile);
    assert(sirenSounded());
  }
}

If you listened closely to your test, it would be asking you, "Why do you need a LocationMobile to test the Siren?" Why indeed? It seems that Sirens shouldn't even know about LocationMobiles.

public class LocationMobile {
  private Alarm alarm;
  public LocationMobile(Alarm alarm) {
    this.alarm = alarm;
  }
  public void alert() {
    alarm.alert(); // alert on Alarm no longer needs a mobile
  }
}

public class TestLocationMobile() extends TestCase {
  public void test_alert() {
    Alarm alarm = EasyMock.createMock(Alarm.class);
    alarm.alert();
    EasyMock.replay(alarm);
    Mobile mobile = new LocationMobile(alarm);
    mobile.alert();
    EasyMock.verify(alarm);
}

It looks like I've just swapped the dependencies. Instead of Alarm depending on Mobile, I now have Mobile depending on Alarm. But if you look closely at the test, the real dependency was Siren knowing about LocationMobile. A concrete class depended on another concrete class. This violates the Dependency Inversion Principle(DIP). The second example has LocationMobile depending on the interface Alarm. A concrete class depending on an abstraction. This satisfies DIP.

If you write all your code TDD, follow the Rule of Simplicity, and Tell Don't Ask then you'll be on the path to becoming a better OO programmer. Good OO code is easy to read and maintain, but can be hard to write. At least at first. The more you write the better you'll become, and the more experience you will get. In the meantime, these practices should get you well on your way.

TDD is a Design Activity

This has been written and talked about before, and if you've read Kent Beck's TDD book, you'll understand how much it's emphasized, but I still see a lot of test code that is about validation and very little about design. And it doesn't need to be that way. Simply follow the three rules of TDD:

1. Only write production code that makes a failing unit test pass.


2. Write the minimum amount of test code to get production code to fail. Not compiling counts.


3. Write only enough production code to get a test to pass.

What I see the most from the test code I read is violations of rule two. Which is very understandable. Why would you not want to write more test code? Because tests, just like every other piece of the system, have three functions:

1. implement the desired feature


2. afford change


3. reveal intent

Writing line after line of test code to cover every possible corner condition certainly meets the first, but violates the second and third. I've seen test code that looks something like:

public void testServicesStopped() {
  assertServicesStoppedIfEnabled(false, false, false, false, false, false);
  assertServicesStoppedIfEnabled(true, false, false, false, false, false);
  assertServicesStoppedIfEnabled(false, true, false, false, false, false);
  assertServicesStoppedIfEnabled(false, false, true, false, false, false);
  assertServicesStoppedIfEnabled(false, false, false, true, false, false);
  assertServicesStoppedIfEnabled(true, true, false, false, false, false);
  assertServicesStoppedIfEnabled(false, true, true, false, false, false);
  assertServicesStoppedIfEnabled(false, false, true, true, false, false);
  assertServicesStoppedIfEnabled(true, true, true, false, false, false);
  assertServicesStoppedIfEnabled(false, true, true, true, false, false);
  assertServicesStoppedIfEnabled(true, true, true, true, false, false);
  // every possible combination was covered
}

Certainly a very thorough test, but also very very intention hiding. Unit tests should function as documentation for the system. You should be able to read the test and determine what a module does and how it does it. To make matters worse, testServicesStopped() was the first method in the test and assertServicesStoppedIfenabled() was near the bottom with many other tests and helper methods in between. To try and figure out what each boolean meant, you had to scroll up and down the file and try to remember 6 parameters. I would also bet a considerable sum that the bulk of this code was written after the production code was completely finished. Not only does this test code not reveal intent, but it's very rigid. It's hard to change because it's hard to understand. When the customer wants to change how this feature works, and the developer opens this test to write the failing test so he can write the feature changes, he'll have a very difficult time figuring out what to change.

Having a suite of unit tests you can run to verify your system still functions is very important, but just as important is having those unit tests clearly document the system. The easiest way to achieve this is to use TDD the way it was designed to be used by following the Three Laws of TDD.

Lazy Inheritance Bugs

I ran across another type of bug yesterday. I'm calling it the Lazy Inheritance Bug. I'm not sure where I first heard the term lazy inheritance, but I've always thought it was a good way to explain the power of delegation, and until yesterday didn't even realize that it could encompass a category of bugs.

Lazy Inheritance

If you've done any object oriented development, you've probably heard the terms is-a and has-a. Is-a is a way to determine if one object should inherit from another. It's not the best criteria to use, but it is a pretty good rule of thumb. Unfortunately, some developers get into the habit of inheriting from another class just to get some reuse. Some method in a class does some work that they need, so they extend that class to get that work for free. That's lazy inheritance.

Lazy Inheritance Bugs

When you use lazy inheritance, you begin a cascading hierarchy that can result in bugs. You extend some class to get some free work. Someone else extends your class to get some of your free work. Someone extends that class to get more free work until you have some methods very far down the hierarchy that depend on methods way up the hierarchy. Now, the author of the original class changes a method and something completely different breaks. That's the Lazy Inheritance Bug.

And that's why terms like is-a exist. To give you some criteria from which to decide if you should inherit from another class. Barbara Liskov provided a better criteria called the Liskov Substitution Principle (LSP). Basically, given a class A that inherits from class B then any class C that has a reference to class A should also work when given a reference to class B without having to know it has a B. In other words, class B should be substitutable for class A. When you use lazy inheritance, you violate the LSP by definition.

Avoiding lazy inheritance bugs is easy: don't use inheritance unless you can conform to the LSP.

Delegation

Before you use inheritance to get reuse or avoid duplication, take some time to consider delegation instead. Here's an example:

Inheritance:

public class Car {
  public void go() {
    // goes pretty fast
  }
}

public class SportsCar extends Car {
  public void go() {
    // goes really fast
  }
}

Notice the only difference between a Car and a SportsCar is the engine. We could use delegation and only have one type of car.

public interface Engine {
  void go();
}

public class EconomyEngine implements Engine {
  public void go() {
    // goes pretty fast
  }
}

public class SportsEngine implements Engine {
  public void go() {
    // goes really fast
  }
}

public class Car {
  private Engine engine;
  
  public Car(Engine engine) {
    this.engine = engine;
  }

  public void go() {
    engine.go();
  }
}

So now you can configure your car with whatever engine you want. This example doesn't seem to hold its weight. There's still inheritance going on, but now it's with Engine and not Car. What gives? Well, just add one more option to see how delegation almost always wins:

public class FamilyCar {
 public void go() {
   // goes pretty fast
 }

 public void turn() {
   // turns OK
 }
}

public class SportsCar {
 public void go() {
   // goes really fast
 }

 public void turn() {
   // turns really well
 }
}

public class SportSuv {
 public void go() {
   // goes really fast
 }

 public void turn() {
   // turns OK
 }
}

public class EconomyCar {
 public void go() {
   // goes pretty fast
 }

 public void turn() {
   // turns really well
 }
}

How can we use inheritance to combine just these two features? With delegation it's easy:

public interface Engine {
 void go();
}

public class SportsEngine implements Engine {
 public void go() {
   // goes really fast
 }
}

public class EconomyEngine implements Engine {
 public void go() {
   // goes pretty fast
 }
}

public interface Suspension {
  void turn();
}

public class SportsSuspension implements Suspension {
  public void turn() {
    // turns really well
  }
}

public class EconomySuspension implements Suspension {
  public void turn() {
    // turns OK
  }
}

public class Car {
  private Engine engine;
  private Suspension suspension;
  public Car(Engine engine, Suspension suspension) {
    this.engine = engine;
    this.suspension = suspension;
  }

  pubic void go() {
    engine.go();
  }

  public void turn() {
    suspension.turn();
  }
}

Now we can configure any Car with any type of Engine and Suspension. When you start thinking about the braking system and traction control and air bags and body style and on and on, you can see that trying to use inheritance for each possible car configuration would get out of hand really fast. Delegation allows you to configure your car with each component and have the reuse in the components and not in the hierarchy. And that's how to squash lazy inheritance bugs before they hatch.

Naming the Flag Object

So, you've decided to avoid the If Bug by using the Flag Object, but you can't seem to come up with a good name. Names should come from natural mappings not artificial ones. By that I mean the names of your objects should come from the customer's not the developers' vocabulary. Listen closely to what the customer says when she describes the story she wants implemented. When I asked my fictional customer,

"How are managers' pay calculated differently from regular employees' pay?"

she said,

"Managers and employees are paid based on different pay scales."

Now, I could have just listened to what she meant and gone about implementing a PayCalculator, but I listened closely to what she said and the words pay scale jumped out at me. That's how PayScale was born. Notice I chose a noun.

Avoid words which do not come from either the customer or the developer domains. "ER" words are the worst: Manager, Calculator, Helper etc.. These words hide the intent of your object, make it hard for other developers to trace its responsibility back to a user story, and lay the groundwork for the object to become large. How many small, cohesive, single responsibility FooManager classes have you ever seen?

When the customer comes to me later and says,

"We've changed the manger's pay scale to take into account how many employees each manager manages,"

I can go straight to the ManagerPayScale object and implement the changes instead of trying to figure out if the change is in the PayManager or EmployeePayCheckUtility or SuperPayRateHelper. The words manager and pay scale were written right in the feature request, so when I look in the paycheck package and see an object called ManagerPayScale chances are I won't have to look much further.

Names are very very important in development. Object names, variable names, and method names can make your code easy to read, intent revealing, and pleasant to work with. On the other hand, poor names hide intent, make code difficult to understand, and painful to maintain. Put effort into choosing your names. And when you get to the refactoring stage, reevaluate your names. Sometimes they make perfect sense when you first came up with them, but later, in the full context of the code, they don't. So change them. If you are not in a situation in which you get to pair program, show your names to other developers. The names might have made perfect sense to you, but others are going to have to maintain your code, so let them have some input.

Name your objects well and not only will other's enjoy maintaining your code, you'll have fewer bugs.

Flag Object

I've worked with a lot of programmers that write if bugs, so I've come up with small simple solutions. The simplest solution I've come up with so far for "if bugs" is what I call the Flag Object.

The Algorithm

1. Instead of making a boolean, make an object.


2. Take the if clause from the client code and make it a method on the object.


3. In the client code call the method on the object.

I said it was simple.

An Example

The Old Code

public class Employee {
private boolean manager;
public boolean isManager() {
  return manager;
}
...
}

...
Employee employee = new Employee();
if (employee.isManager()) {
// calculate manager pay
} else {
// calculate employee pay
}
...

This is a potential "if bug". The following is better and only slightly more 'work':

public class Employee {
 private PayScale payscale;
 public double calculatePay() {
   payscale.calculatePay();
 }
...
}

public interface PayScale {
 double calculatePay();
}

public EmployeePayScale implements PayScale {
 public double calculatePay() {
   // calculate employee pay
 }
}

public ManagerPayScale implements PayScale {
 public double calculatePay() {
   // calculate manager pay
 }
}

...
Employee employee = new Employee()
double pay = employee.calculatePay()
..

If you were doing the old boolean flag trick, you'd have places in the code to set the boolean true or false.

employee.setManager(true);

Now you call something like:

employee.setPayScale(new ManagerPayScale());

Now the details of how a manager's and employee's pay is calculated is encapsulated in the PayScale objects. How pay is calculated can change and not affect anyone who actually uses the code to calculate pay. You can also add new employee types and not have to change existing code. That means the code satisfies the Open-Closed Principle.

There a few things I don't like. One, the object that calls

employee.setPayScale()

depends on the low level objects in the PayScale hierarchy. This breaks the Dependency-Inversion Principle that says concrete objects should depend on abstractions. Preferably the employee object is obtained from some kind of factory that knows how to make Employee objects already configured with the appropriate PayScale object. However, it's better than the "if bug" problem so I can live with a little dependency issue. At least the dependency issue is localized and easily solved. If Bugs can be spread throughout the code and finding all of them very very difficult. I also prefer Flag Objects because they conform to Tell Don't Ask.

If Bugs

I've run across a particularly nasty type of bug, and I really haven't seen anyone else talk about it, so I will. I call them "if bugs" and they occur when encapsulation is broken. They usually take the form of:

public class Employee {
 private boolean manager;

 public boolean isManager() {
  return manager;
 }
...
}

...
if (employee.isManager()) {
 calculateManagerPay();
else {
 calculatePay();
}
...

The problem is that now there are potentially many other parts of the system that care about whether an employee is a manager or not. Some of those other parts will not be changed, and defects will be reported against the system when those parts of the application are used. That's the "if bug".

Encapsulation was broken when manager was exposed to the rest of the system. Object Oriented Design tells us to not expose the implementation details of a class to the rest of the system.

There are many better ways to implement this, and I'll save what is perhaps the simplest way, Object Flag, for next time.