I have this problem called out by PMD (static code analyzer) more often that I would like, and would like to know how someone with more experience than me would write it. My code is functional but I find it inelegant, so I want to know how other programmers would write this piece.
The case is, in a network/IO petition I may or may not get a result from, but my parent method is not null-proof so I always have to return something. I also don’t like several returns on a method.
public String getBingLocation(Coordinate... data)
{
String response = "Not Retrieved";
final Coordinate location = data[0];
JSONObject locationData;
try {
locationData = NetworkManager.getJSONResult(ApiFormatter
.generateBingMapsReverseGeocodingURL(location.Latitude, location.Longitude));
if (null != locationData) {
final String address = this.getAddressFromJSONObject(locationData);
response = address;
}
} catch (final ClientProtocolException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
return response;
} catch (final JSONException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
return response;
} catch (final IOException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
return response;
} finally {
location.Street = response;
}
return response;
}
Other example:
public static Object loadObject(final String fileName, final Context context) {
Object object = null;
try {
ObjectInputStream objectInputStream = null;
try {
final FileInputStream fileStream = context.openFileInput(fileName);
objectInputStream = new ObjectInputStream(fileStream);
object = objectInputStream.readObject();
} catch (final ClassNotFoundException catchException) {
LoggerFactory.consoleLogger().printStackTrace(catchException);
} catch (final ClassCastException catchException) {
LoggerFactory.consoleLogger().printStackTrace(catchException);
} catch (final Exception catchException) {
LoggerFactory.consoleLogger().printStackTrace(catchException);
} finally {
if (objectInputStream != null) {
objectInputStream.close();
}
}
} catch (final IOException catchException) {
LoggerFactory.consoleLogger().printStackTrace(catchException);
}
return object;
}
3
Part of the problem is your functions have more than one responsibility. They are both getting a result and handling errors. You’re also getting unnecessarily hung up trying to avoid multiple return points, which is a vestigial practice from C programming where it can cause memory leaks. You should use multiple returns if it clarifies your code, especially in short functions. I’m also guessing that your logging code is getting repeated all over the place. If you separate those concerns and try to avoid repeating yourself, you get something like this:
public JSONObject getLoggedJSONResult(String api)
{
try {
return NetworkManager.getJSONResult(api);
} catch (final ClientProtocolException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
} catch (final JSONException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
} catch (final IOException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
}
return null;
}
public String getBingLocation(Coordinate... data)
{
final Coordinate location = data[0];
JSONObject locationData = getLoggedJSONResult(
ApiFormatter.generateBingMapsReverseGeocodingURL(
location.Latitude, location.Longitude));
if (null == locationData) {
return "Not Retrieved";
} else {
return this.getAddressFromJSONObject(locationData);
}
}
As a general principle, most DD anomalies can be fixed by splitting the function. It’s mostly a matter of figuring out where to split it.
Your second example is trickier. That sort of situation is why try-with-resources statements were invented. You can use an early return to eliminate the DD anomaly on object, but you’re not going to be able to do much about objectInputStream without a try-with-resources.
3
As for your first example, I’d write it like this :
public String getBingLocation(Coordinate... data)
{
String response;
final Coordinate location = data[0];
JSONObject locationData;
try {
locationData = NetworkManager.getJSONResult(ApiFormatter
.generateBingMapsReverseGeocodingURL(location.Latitude, location.Longitude));
if (null != locationData) {
final String address = this.getAddressFromJSONObject(locationData);
response = address;
} else {
response = "Not Retrieved";
}
} catch (final ClientProtocolException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
response = "Not Retrieved";
} catch (final JSONException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
response = "Not Retrieved";
} catch (final IOException e) {
LoggerFactory.consoleLogger().printStackTrace(e);
response = "Not Retrieved";
} finally {
location.Street = response;
}
return response;
}
The main idea is to avoid overriding a value without reading it (this means one assignement is useless because never read).
A solution is to have a single return statement, and return a non initialized variable. Then go through all possible path and check that a value is always assigned, and no more than once.
8