Resolving paths

Question

We're using a class called Places (inspired by Allen Holub's great article on DrDobbs) that resolved our program paths based on the existence of a configuration file, environment variable or neither. In the neither case, we want all paths to default to resources within our package, the .jar. Additionally, we want to force local paths if we are in a testing environment because of the complexity in handling the Lucene Database.

The original code looks like this:

private void resolvePaths() {
    for (NewPlaces place : values()) {
        List<String> probablePaths = new ArrayList<>();

        try {
            if (! IS_IN_A_UNIT_TESTING_ENVIRONMENT && !place.forceLocalPaths) {
                File configuration = new File("places.properties");

                if (configuration.exists()) {
                    LOGGER.info("The configuration file exists!");
                    Properties properties = new Properties();
                    properties.load(new FileInputStream(configuration));

                    probablePaths.add(place.path.replace("~", properties.getProperty("mediaplatform.home")));
                }

                if (System.getenv("MEDIAPLATFORM_HOME") != null) {
                    String environmentValue = System.getenv("MEDIAPLATFORM_HOME");

                    probablePaths.add(place.path.replace("~", environmentValue));
                }
            }
        } catch (IOException e) {
            throw new RuntimeException("The file could not be read.");
        }

        String localPath = place.path.replace("~/", "");
        URL localUrl = Thread.currentThread().getContextClassLoader().getResource(localPath);
        if (localUrl != null)
            probablePaths.add(localUrl.getPath());

        for (String probablePath : probablePaths) {
            File file = new File(probablePath);
            if (file.isDirectory()) {
                LOGGER.info("\t{} -> {}", place.path, probablePath);
                place.path = probablePath;
                break;
            }
        }

        if (place.path.contains("~"))
            throw new RuntimeException("Could not find a proper path for value '" + place.path + "'");
    }
}

I thought this function was long and needlessly complex to read, so I tried refactoring it into smaller methods with clearer meanings. The code ended up roughly 30 lines longer. The question is, did I make it simpler, easier to read and maintain, or did I just waste my time?

private void resolvePaths() {
    for (NewPlaces place : values()) {
        List<String> probablePaths = new ArrayList<>();
        if (!IS_IN_A_UNIT_TESTING_ENVIRONMENT)
            probablePaths.addAll(findExternalPaths(place.path));
        probablePaths.add(resolveLocalPath(place.path));

        place.path = findBestPath(probablePaths);
    }
}

private String resolveLocalPath(String unresolvedPath) {
    String localPath = unresolvedPath.replace("~/", "");
    URL localUrl = Thread.currentThread().getContextClassLoader().getResource(localPath);

    checkArgument(localUrl != null, "The path '%s' does not exist locally, but it should.", localPath);

    return localUrl.getPath();
}

private List<String> findExternalPaths(String unresolvedPath) {
    List<String> externalPaths = new ArrayList<>();

    Optional<String> configurationPath = resolvePathFromConfiguration(unresolvedPath);
    if (configurationPath.isPresent())
        externalPaths.add(configurationPath.get());

    Optional<String> environmentPath = resolvePathFromEnvironment(unresolvedPath);
    if (environmentPath.isPresent())
        externalPaths.add(environmentPath.get());

    return externalPaths;
}

private Optional<String> resolvePathFromConfiguration(String unresolvedPath) {
    File configuration = new File("places.properties");
    if (!configuration.exists())
        return Optional.absent();

    try {
        LOGGER.info("The configuration file exists!");
        Properties properties = new Properties();
        properties.load(new FileInputStream(configuration));

        return Optional.of(unresolvedPath.replace("~", properties.getProperty("mediaplatform.home")));
    } catch (IOException e) {
        throw new RuntimeException("Could not open or access configuration file 'places.properties'");
    }
}

private Optional<String> resolvePathFromEnvironment(String unresolvedPath) {
    if (System.getenv("MEDIAPLATFORM_HOME") == null)
        return Optional.absent();

    String environmentValue = System.getenv("MEDIAPLATFORM_HOME");
    return Optional.of(unresolvedPath.replace("~", environmentValue));
}

private String findBestPath(List<String> probablePaths) {
    for (String probablePath : probablePaths) {
        File file = new File(probablePath);

        if (file.isDirectory())
            return probablePath;
    }

    throw new RuntimeException("Could not find a proper path!");
}

Additional Info:

NewPlaces is an enum class with the following structure:

public enum NewPlaces {
    DATA_STORE_PERSON("~/data_stores/prod/person_store"), DATA_STORE_TERM("~/data_stores/prod/term_store"), DATA_STORE_RESOURCE(
            "~/data_stores/prod/resource_store"),

    TESTDATA("~/testdata", ForceLocalPaths.TRUE), PROPERTIES("~/properties"), CONFIGURATION("~/configuration", ForceLocalPaths.TRUE);

    private enum ForceLocalPaths {
        TRUE, FALSE;
    }

If a developer tries to access a directory contained in an enum for the first time, then the paths will be resolved via the above semantics:

public File directory() {
    if (path.contains("~"))
        resolvePaths();

    File directory = new File(path);
    assert directory.isDirectory() : "The directory does not exist or is not a directory.";

    return directory;
}

Answer 1

Due to the size of the code I think you'd be better off just adding some comprehensive comments rather than refactor.

So to answer your question:

Did you needlessly complicate it? Yes and No

Yes: It is needless since the code already worked as intended.
No: You didn't complicate it.

So you spent some time for an un-necessary refactor, but if it helped you understand the code then that is a plus.

Answer 2

You're on the right track, you just need to take it a bit further.

• The IS_IN_A_UNIT_TESTING_ENVIRONMENT is the greatest eye sore. Delegate loading properties from a file to a collaborator class and the code will become simpler in this class, and easier to test, since you can now inject a dummy/mock PropertiesLoader.

example - untested -

public interface PropertiesLoader {

    Optional<Properties> load(String name);
}

public class FilePropertiesLoader implements PropertiesLoader {
    @Override
    public Optional<Properties> load(String name) {
        File configuration = new File(name + ".properties");
        if (!configuration.exists()) {
            return Optional.absent();
        }
        LOGGER.info("The configuration file exists!");
        Properties properties = new Properties();
        try {
            properties.load(new FileInputStream(configuration));
            return Optional.of(properties);
        } catch (IOException e) {
            throw new RuntimeException(e);
        }
    }
}

• Optional is a great guava class, but probably a bit overkill to use in private methods.

• a few more extract methods to work away some duplication, and some tactical renames can make all the difference.

• the result is perhaps a few lines longer, but you'll find that shorter methods help you in all kinds of ways :

  • they make code more readable
  • they separate different levels of abstraction
  • in case of a bug, the stacktrace will be much more revealing what is going wrong
  • they offer more opportunity to explain what the code is doing in method names rather than in comments, which are harder to keep in sync with the code.
  • I find small methods can be categorized in a number of idioms, this makes them easy to recognize, and lowers their conceptual threshold.

And of course : one large method is always a puzzle.

my current version :

private void resolvePaths() {
    for (NewPlaces place : NewPlaces.values()) {
        resolvePathsForPlace(place);
    }
}

private void resolvePathsForPlace(NewPlaces place) {
    List<String> probablePaths = new ArrayList<>();
    probablePaths.addAll(findExternalPaths(place.path));
    probablePaths.add(resolveLocalPath(place.path));
    place.path = findBestPath(probablePaths);
}

private String resolveLocalPath(String unresolvedPath) {
    String localPath = unresolvedPath.replace("~/", "");
    URL localUrl = Thread.currentThread().getContextClassLoader().getResource(localPath);
    checkArgument(localUrl != null, "The path '%s' does not exist locally, but it should.", localPath);
    return localUrl.getPath();
}

private List<String> findExternalPaths(String unresolvedPath) {
    List<String> externalPaths = new ArrayList<>();
    addIfNotNull(externalPaths, resolvePathFromConfiguration(unresolvedPath));
    addIfNotNull(externalPaths, resolvePathFromEnvironment(unresolvedPath));
    return externalPaths;
}

private void addIfNotNull(List<String> externalPaths, String configurationPath) {
    if (configurationPath != null) {
        externalPaths.add(configurationPath);
    }
}

private String resolvePathFromConfiguration(String unresolvedPath) {
    Optional<Properties> properties = propertiesLoader.load("places");
    return properties.isPresent() ? unresolvedPath.replace("~", properties.get().getProperty("mediaplatform.home")) : null;
}

private String resolvePathFromEnvironment(String unresolvedPath) {
    if (System.getenv("MEDIAPLATFORM_HOME") == null) {
        return null;
    }
    return unresolvedPath.replace("~", System.getenv("MEDIAPLATFORM_HOME"));
}

private String findBestPath(List<String> probablePaths) {
    for (String probablePath : probablePaths) {
        if (new File(probablePath).isDirectory()) {
            return probablePath;
        }
    }
    throw new RuntimeException("Could not find a proper path!");
}


private void checkArgument(boolean b, String s, String localPath) {
    //To change body of created methods use File | Settings | File Templates.
}

Answer 3

I think you have achieved your objective. The new code is significantly easier to follow and will be much easier to maintain.