Exposing configuration elements to program

I have a config file stored in a java properties file:

output_path=/some/path/somewhere
num_threads=42
# ad infinitum

I have an singleton enum which controls the building of the config:

public enum ConfigFactory {
    INSTANCE;

    public static final String OUTPUT_PATH = "output_path";
    public static final String NUM_THREADS = "num_threads";

    private Map mSettings = new HashMap();

    public void build(String path) {
        InputStream inputStream = getClass().getResourceAsStream(path);
        Properties properties = new Properties();
        try {
            properties.load(inputStream);
            inputStream.close();
            for(Map.Entry entry : properties.entrySet()) {
            // for brevity I only look for ints    
            if(StringUtils.isNumeric(entry.getValue().toString())) {
                    mSettings.put(entry.getKey().toString(), new ConfigParameter((Integer) entry.getValue()));
                } else {
                    mSettings.put(entry.getKey().toString(), new ConfigParameter(entry.getValue().toString()));
                }
            }
        } catch(IOException e) {
            // intentionally left empty
        }
    }

    public ConfigParameter getValue(String s) {
        return mSettings.get(s);
    }

    private class ConfigParameter {
        private T mConfigParameter;

        public ConfigParameter(T var) {
            mConfigParameter = var;
        }

        /**
         * Set the value of the parameter
         * @param param Generic object to be stored
         */
        public void set(T param) {
            mConfigParameter = param;
        }

        /**
         * Retrieve the object
         * @return Generic object stored
         */
        public T get() {
            return mConfigParameter;
        }
    }
}

Elsewhere in my program, when I want to obtain a config element, I reference it this way:

```
int numberOfThreads = ConfigFactory.INSTANCE.getValue(ConfigFactory

Encapsulation

It's not really necessary that you expose these at all:

public static final String OUTPUT_PATH = "output_path";
public static final String NUM_THREADS = "num_threads";

Let the ConfigFactory be responsible for that, and save the actual values as instance variables, instead of using public ConfigParameter getValue(String s).

For example, instead of exposing getValue(String s), expose these:

public int getNumThreads() {
    ...
}

public String getOutputPath() {
    ...
}

Where the implementation of those methods could either be return getValue(SOME_KEY); or return this.numThreads; , if you add such private fields that you initialize after you've read the config file.

Testability, Dependency Injection

Why is this even a singleton? It's good that you have a method that takes a path so that you can use different configs, but your code leaves some things to be desired.

-
Immutability / Concurrency: It is currently possible that one thread reads a value, while another thread calls the build method for the Nth time.

-
Generics: ConfigParameter is using generics, but your getValue method simply returns ConfigParameter, you could change that to:

public  ConfigParameter getValue(String s)

-
Mocking / Testing: If you would want to write unit tests (which you of course want to do), you would have to actually read a configuration file to be able to use the config. However, if you extract an interface, that exposes the important methods, such as getNumThreads() then you could easily create other implementations of this interface, which doesn't even have to read from a config file!

Additionally, instead of a enum I would use a static factory method.

Here's how to use it as an static factory method, although I hope you will take my other advice as well about extracting an interface etc:

public class MyConfig { // it's not a factory

    private Map mSettings = new HashMap();

    public static MyConfig build(String path) {
        MyConfig config = new MyConfig();
        InputStream inputStream = getClass().getResourceAsStream(path);
        Properties properties = new Properties();
        try {
            properties.load(inputStream);
            inputStream.close();
            for (Map.Entry entry : properties.entrySet()) {
            // for brevity I only look for ints    
            if(StringUtils.isNumeric(entry.getValue().toString())) {
                    config.mSettings.put(entry.getKey().toString(), new ConfigParameter((Integer) entry.getValue()));
                } else {
                    config.mSettings.put(entry.getKey().toString(), new ConfigParameter(entry.getValue().toString()));
                }
            }
            return config;
        } catch(IOException e) {
            return null; // return null to indicate that a problem occurred
            // although the best thing would be to throw an exception here
        }
    }

    // ... ConfigParameter and other stuff
}

StackExchange Code Review Q#105300, answer score: 4