Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

###Nitpicks

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 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 :)

###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 :)

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 :)

Source Link
Vogel612
  • 25.5k
  • 7
  • 59
  • 141

###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 :)

lang-java

AltStyle によって変換されたページ (->オリジナル) /