Reading properties from file during standalone application startup

I've a standalone Java application, which basically starts and manages two socket servers. I'd like to configure server ports in a .properties file using the following class.

class ApplicationConfig {

    private static final Logger     LOG            = Logger.getLogger(ApplicationConfig.class.getName());
    private static final Properties APP_PROPERTIES = new Properties();

    static int defaultEventServerPort  = 9090;
    static int defaultClientServerPort = 9099;

    static {
        try {
            APP_PROPERTIES.load(ClassLoader.class.getResourceAsStream("/app.properties"));
            String eventServerPort = APP_PROPERTIES.getProperty("server.event.port");
            String clientServerPort = APP_PROPERTIES.getProperty("server.client.port");

            if (isValidNumeric(eventServerPort)) {
                defaultEventServerPort = Integer.valueOf(eventServerPort);
            }
            if (isValidNumeric(clientServerPort)) {
                defaultClientServerPort = Integer.valueOf(clientServerPort);
            }
        } catch (IOException ex) {
            LOG.log(
                    Level.WARNING,
                    "Unable to load server ports from properties file, going to use default port {0} for event server and {1} for client server",
                    new Object[]{defaultEventServerPort, defaultClientServerPort}
            );
        }
    }

   private static boolean isValidNumeric(String v) {
        if (v == null || v.length() == 0) {
            return false;
        }
        for (int i = 0; i < v.length(); i++) {
           if (!Character.isDigit(v.charAt(i))) {
              return false;
           }
        }
        return true;
   }
}

I really hate ApplicationConfig class, the static initialization block bothers me, but I am not able to find a better idea yet. How you would suggest to modify it?

And here is my main class

```
public class Application {

private static final Logger LOG = Logger.getLogge

What I find bothering is not the ApplicationConfig class and the static initializer, in itself. It's the fact that defaultEventServerPort and defaultClientServerPort are global variables, which should be avoided.

What you should have are two constants defining the default values, 2 variables holding the actual values as instance fields.

private static final int DEFAULT_EVENT_SERVER_PORT = 9090;
private static final int DEFAULT_CLIENT_SERVER_PORT = 9099;

private int eventServerPort;
private int clientServerPort;

and the initialization of those 2 variables done in the constructor. The purpose of doing this in the constructor, instead of a static initializer, is that it lets you have proper instance fields.

The first concern is that there's duplicated code in this: the logic for extracting the port from the Properties object, converting it to an int or using the default value, will be the same for both ports. Therefore, it makes sense to create a method for that:

private static int getAsIntOrDefault(Properties properties, String key, int defaultValue) {
    String val = properties.getProperty(key);
    if (isValidNumeric(val)) {
        return Integer.parseInt(val);
    }
    return defaultValue;
}

Notice that instead of using Integer.valueOf, which returns an Integer object, we can directly use Integer.parseInt, which returns a primitive int. This way, we don't need to have a boxing, and then unboxing conversion.

There is another problem with how the Properties object is loaded: you have a potential memory leak!

APP_PROPERTIES.load(ClassLoader.class.getResourceAsStream("/app.properties"));

This is opening an InputStream, but it is never closed. Instead, use a try-with-resources construct:

try (InputStream is = ClassLoader.class.getResourceAsStream("/app.properties")){
    APP_PROPERTIES.load(is);
} catch (IOException ex) {
    // ...
}

With those changes, you can have the following:

ApplicationConfig() {
    try (InputStream is = ClassLoader.class.getResourceAsStream("/app.properties")){
        APP_PROPERTIES.load(is);
    } catch (IOException ex) {
         // do the logging
    }
    eventServerPort = getAsIntOrDefault(APP_PROPERTIES, "server.event.port", DEFAULT_EVENT_SERVER_PORT);
    clientServerPort = getAsIntOrDefault(APP_PROPERTIES, "server.client.port", DEFAULT_CLIENT_SERVER_PORT);
}

The try-catch block was also reduced so that it spans the minimal amount of code as possible. try-catch should be small and cover only the part of the code that can actually throw the exception you want to catch.

Last point, you can see the advantage of using a constructor here: later, you may want to remove hard-coding "/app.properties". With a constructor, you can easily now pass the path to the properties file, which would be complicated to do with a static initializer.

Nitpick: isValidNumeric won't return true for negative numbers, so you may want to rename it to isValidPositiveInteger. Also, does APP_PROPERTIES really need to be static final as well? I would imagine it is only used to fetch all of the configuration values once, and unused later on.

StackExchange Code Review Q#147673, answer score: 4