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