Reading properties from file during standalone application startup

Question

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.getLogger(Application.class.getName());
 private Application() {}
 public static void main(String[] args) {
 try {
 ExecutorService pool = Executors.newCachedThreadPool();
 EventServer eventServer = new EventServer(new Configuration(ApplicationConfig.defaultEventServerPort));
 ClientServer clientServer = new ClientServer(new Configuration(ApplicationConfig.defaultClientServerPort));
 pool.submit(eventServer);
 pool.submit(clientServer);
 } catch (IOException e) {
 LOG.log(Level.SEVERE, "Unable to start servers", e);
 }
 }
}

• Mario Santini: You should not override the default parameters. Instead, you should have defaults and other 2 parameters for actual values. and then have a method that returns the actual value.
• Vogel612: @MarioSantini that's a review you got there ... add some more meat to it and post it as an answer, please :)
• Vogel612: the declaration of isValidNumeric could be pretty interesting, because the way you use it you may be doing a lot of needless work.
• Mario Santini: @Vogel612 I wrote as a comment because the OP asked for an alternative of the configuration class, and my wasn't connected to that point.
• Vogel612: @MarioSantini reviewers may address any point they want about the code presented. This is especially true, because the reviewee may not know what they actually need (as opposed to what they think they need). Write that review, you got my upvote (because there's more to find than just that). For more info on that policy you may want to check this meta post.

Answer 1

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.

• vtor: Great suggestions, appreciate your effort.

Answer 2

Nitpicks

Formatting & Use of API:

• LOG has some very strange whitespace there. Why not simply write
  private static final Logger LOG = Logger.getLogger(ApplicationConfig.class.getName());?
• The Logger#log method's third argument is a vararg. This means that instead of explicitly creating a new Object[] you can just skip that and call
  LOG.log(Level.WARNING, "message", defaultEventServerPort, defaultClientServerPort);
• On that note: I personally prefer to have the first argument of a method call on the same line as the call. Also I don't like closing the parens on a separate line. But that's personal preference, ignore it at your discretion :)

Needless Work:

The code is doing some unnecessary work in the Constructor. For one it uses Integer.valueOf(String), which calls Integer.parseInt(String) and returns a boxed Integer object that is then unboxed and assigned to a primitive int.

Additionally your isValidNumeric could possibly benefit from using a touch of Java Streams:

private static boolean isValidNumeric(String v) {
 if (v == null || v.length() == 0) {
 return false;
 }
 return v.codePoints().allMatch(Character::isDigit);
}

While this shouldn't give any performance gains, it's IMO significantly easier to mentally process.

Terminating the Application / UX

Terminating your application is a little difficult. For one the ExecutorService used for the Servers is going out of scope as soon as main ends. This makes it needlessly hard to cleanly shut down on certain commands.

Additionally I'd expect to be able to pass the path to a configuration file to my executable so I can decide which configuration I want to go with. This has the added benefit of not needing to repack a jar every time I change configuration. This would necessitate a different loading mechanism for the configuration, since it can't be carried over in the static scope. Incidentally this resolves your grievance with the static initializer block as a side-effect :)

Answer 3

Other answers are very good regarding your question.

There isn't an issue yet in your code, but I found something I think you should do with the catch block: it is a very good idea to enforce going into a well-defined state in a catch block that logs an exception then continues.

---

In your case, if an exception occurs during the setup of the client port, after the events port has been setup, anywhere here:

if (isValidNumeric(clientServerPort)) {
 defaultClientServerPort = Integer.valueOf(clientServerPort);
}

Then the program will jump to the catch block, but keep the events port from the properties file, and the client port default. Both values will probably not make sense together.

If ever the method becomes a utility method, gains a throws statement, or whatnot, then you'll get into a incoherent state (though the logger would correctly log that state).