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. Because it is spike code there are a couple of notable differences from good development practices that I should point out. First, I wrote this code alone. No pair programming and no code review. Second, I wrote no tests. Not first, not during, not after. Not doing TDD is why I believe I created such a horrible mess in the first place which then allowed me to refactor it here, for your enjoyment.

Here's the original run() method 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. Code like this should make you at least a little queezy.

Methods and Abstraction

I learned from Clean Code by Robert Martin that all methods should have the same level of abstraction. 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);
...

Now, run() has code that looks like:

...
writeFileDetails(fileDetails, directoryName);
directoryName = reader.readLine();
...

writeFileDetails(fileDetails, directoryName); looks a little out of place with directoryName = reader.readLine(); on the next line. Those two lines are not at the same level of abstraction. writeFileDetails is a more abstract concept. How the details are written is not revealed. reader.readLine() is much less abstract since it is referencing a FileReader and exposes the actual mechanism being used.

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;
    }
}