anathema/anathema — pull request #204 by curttasker (2012-04-26 06:39:29+00:00)
Title: Fixes two null pointer exception crashes in the current build:

- Repository folder does not exist
- Repository folder exists, but "custom" child folder does not exist.

This fix uses the same type of code that `RepositoryPreferencesElement` uses, simply because I knew it worked and it was easy to cut and paste. That said, its crazy verbose and amounts to doing with a wall of text what one single function could.

@UrsKR your new code was using `RepositoryLocationResolver` to retrieve the pathname of the repository. That class has logic in it simply to sort through the system properties and Preferences budle and figure out where the default repository location and current repository location is. It deals entirely in Strings, it knows nothing about files.

So the code was simply creating a new File object from the nonexistent (on my system) `repository/custom` folder, and causing null pointer exceptions if it was not. Further testing proved it did the same thing if the `repository` folder itself was nonexistent.

While fixing this bug, I realized how **bad** our code is for retrieving (and creating, if necessary) the repository folder, and its child folders. I had some warning bells going off in my head when writing the new repository folder selection feature, but I didn't fully grasp how bad things had gotten. It looks like you're working on cleaning this up, but there are a few parts missing, as I found tonight while poking around.

## How to do it

Remember that a File object isn't guaranteed to exist, its simply an abstract pathname. You have to specifically test for its existance with `.exists()`, and then handle creating the folder/file if you need it to exist. You can create the file using `mkdir()`, or create a `FileWriter`, which will create implicitly if the pathname does not exist. The simplest method, however, combines `exists()` and `mkdir()`:

file.createNewFile();

That function will only create the file if it does not allready exist. It returns true if the file did not exist (but now does), and false if it already existed.

## How we're handling it

The actual code that checks for the repository folder, and creates it if necessary, is not in any single place. When dealing with `RepositoryPreferencesElement` for my recent feature addition, I took the existing code and beefed it up a little, but I tried to keep the same structure in place. Honestly, its a nightmare. Abstract pathnames are created in the constructor, and checked/created in `isValid()` and `isDefaultValid()`, and they use dynamically created `IStringResolver` classes and `RepositoryFolderCreator` to handle making sure the folder exists. The code looks like this:

IOFileSystemAbstraction fileSystem = new IOFileSystemAbstraction();
new RepositoryFolderCreator(fileSystem, new IStringResolver() {
@Override
public String resolve() {
try {
return repositoryDirectory.getCanonicalPath();
} catch (IOException e) {
throw new RepositoryException("Could not resolve path of default directory");
}
}
}).createRepositoryFolder();

The `RepositoryFolderCreator.createRepositoryFolder()` called at the end of that block is here:

public File createRepositoryFolder() throws RepositoryException {
File file = new File(pathResolver.resolve());
if (!fileSystem.exists(file)) {
try {
fileSystem.createFolder(file);
}
catch (IOException e) {
throw new RepositoryException(e);
}
}
if (!fileSystem.canRead(file) || !fileSystem.canWrite(file)) {
throw new RepositoryException("Read/Write error on repository at " + file.getAbsolutePath()); //$NON-NLS-1$
}
return file;
}

`IOFilesystemAbstraction` is an abstraction on top of `File`, and I'm not really sure why its existence is even justified. In any case, its houses the folder creation routine that the above code calls, and it in turn passes the actual folder creation task off to `org.apache.commons.io.FileUtils`.

`Repository` handles creation of the folder as well, but does simply (and slightly incorrectly, it need not call `exists()` in this case.):

if (!file.exists()) {
try {
file.createNewFile();

`PropertiesMatcher` uses the `FileWriter` to create its directories, which will create them if they do not already exist.

`RepositoryFileResolver` uses `mkdir()`, and doesn't check for failure and warn the user.

## Thoughts
So basically I count at least 3 places right now that are handling creation of the repository folder, nevermind the subfolders. There obviously should be only one, since we don't have a guaranteed order of execution for a lot of these bits of code, so we can't rely on other places creating the repository folder for us.

It looks like you've made some headway in your cleanup in reducing them. I'm guessing that eventually there'll be one place where we can `Repository.getLocation()` and `Repository.getFile()` and `Repository.getResource("custom")`` and the like.