SpotBugs analysis: Possible NPE dereference in getPort()

[ghost]

From SpotBugs:

The problem appears to be in the following code:

public int getPort() {
        return Util.isValidNetworkPort(getParameters().get("port"), true) ? Integer.parseInt(getParameters().get("port")) : null;
}

Calling getParameters() is a minor violation of encapsulation (because it leaks API details about how parameters are stored). Since all *BusAddress.java classes extend from BusAddress, consider exposing get*Parameter() and setParameter() methods (and the like) that hide the implementation details about how the parameter properties are stored. Consider:

private static final String PARAM_HOST = "host";
private static final String PARAM_PORT = "port";

public String getHost() { 
  return getParameter(PARAM_HOST);
}

public int getPort() {
  return getIntParameter(PARAM_PORT);
}

public boolean hasHost() {
  return hasParameter(PARAM_HOST);
}

public boolean hasPort() {
  return hasParameter(PARAM_PORT);
}

public int getPort() {
  final var port = getPort();
  return Util.isValidNetworkPort(port) ? port : null;
} 

From there, it may be beneficial to force a default, such as:

public int getPort(final int defaultPort) {
  final var port = getPort();
  return Util.isValidNetworkPort(port) ? port : defaultPort;
} 

Of course, if a default port isn't suitable, then maybe an Optional would work?

This makes the code a little more null-hostile while addressing the potential NPE reported by SpotBugs and eliminating the duplicate/redundant calls to getParameters().

[hypfvieh]

I see that a default port would be useful. I don't see what the other changes are good for.

Exporting the parameter Strings to constants will change exactly nothing as the constants will be inlined during compile time. Also the constants are not used multiple times, so I don't see any benefit in extracting the Strings to constants.

The BusAddress class is designed to be used for any transport, so changing that one may break existing transports including custom transport I don't know of. It may be possible to allow better encapsulation in the next major version but I don't see a real issue here. BusAddress and all sub classes are used to contain addressing information to connect to dbus. It was designed to allow containing arbitrary data (because I don't know which information might be needed for transports different to unix socket / tcp). If a user thinks it is a good idea to mess with the content of the underlying map during some operation, it may result in problems connecting to dbus. Any additional stuff placed in the map will (at least in the transports provided) do no harm, as they are not read.

[ghost]

Exporting the parameter Strings to constants will change exactly nothing as the constants will be inlined during compile time.

Constants have many advantages for developers:

• Prevents accidental typos.
• Ensures case matches consistently.
• Allows control+click in the IDE to find all references to the parameter name.

Encapsulation is a good idea, generally.

To each their own though. :-) Thanks for fixing the NPEs!

[hypfvieh]

Prevents accidental typos. Ensures case matches consistently. Allows control+click in the IDE to find all references to the parameter name.

Yes it prevents typos, but as these few Strings are only used internally in TcpBusAddress, nobody needs to type it anywhere. Same goes for case matching. Finding references is only useful if the constants could be used outside of that class, which is not the case.

Anyway, I updated the code once more to use better encapsulation and deprecated the getParameter() method.