6
\$\begingroup\$

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<String, ConfigParameter> mSettings = new HashMap<String, ConfigParameter>();

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

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

    private class ConfigParameter<T> {
        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.NUM_THREADS).get();

This method can be seen as untenable as I need to have static string literals for each config value defined in the properties file. Is there a better way to expose these config values to the program and have only one place to change them if the properties file changes? Is this considered best practices for exposing properties values to the program?

\$\endgroup\$
5
\$\begingroup\$

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 <T> ConfigParameter<T> 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<String, ConfigParameter> mSettings = new HashMap<String, ConfigParameter>();

    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<Object, Object> 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>((Integer) entry.getValue()));
                } else {
                    config.mSettings.put(entry.getKey().toString(), new ConfigParameter<String>(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
}
\$\endgroup\$
  • \$\begingroup\$ I thought about using access/mutator methods as well. But I shot that down in favor of this approach because of what happens when there are 15/25/50/etc config elements. As for "why is this even a singleton, it is because there only ever needs to be one config object in the system. So threading is rather irrelevant on this point. You build it once and the config is now accessible everywhere in the system. I like the idea of interfacing it, but if for the sole purpose of testing, what do I actually gain? I appreciate the input and will use it, I'm asking for understanding and learning. \$\endgroup\$ – lilott8 Sep 21 '15 at 20:24
  • \$\begingroup\$ @lilott8 Then you'll either have 50 String constants, 50 enum constants, or 50 accessors. While you should consider lines of code when comparing these solutions, it probably shouldn't be your primary consideration. Not all lines of code are created equal: accessors add zero mental load while introducing several benefits. \$\endgroup\$ – DavidS Sep 21 '15 at 20:35
  • 1
    \$\begingroup\$ @DavidS, that's fine. That was a very poorly phrased statement; the question that was behind that statement was: "Is there no way to get around the 15/25/50 variables/enums/accessors?" And the answer is "no, not without trading other attributes (mainly encapsulation)". \$\endgroup\$ – lilott8 Sep 21 '15 at 20:40
  • \$\begingroup\$ @lilott8 "You build it once and the config is now accessible everywhere in the system." that tells me that it should be immutable, which your current code is not. Using a static factory method is one way to make it immutable. Making a true singleton, that is also immutable is not an easy task. Your current approach is a singleton, but not immutable. My static factory method makes it immutable, but not a singleton. Does it really need to be a singleton? I would use the approach of creating the config object and then pass it to every object that requires it. \$\endgroup\$ – Simon Forsberg Sep 22 '15 at 6:22
  • 1
    \$\begingroup\$ @lilott8 See also: Tell, don't ask \$\endgroup\$ – Simon Forsberg Sep 22 '15 at 6:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.