Looking up a hostname for a given path and client ID

Question

I have this code in my library and I wanted to see whether we can avoid duplicating stuff.

private String getAddress(final String path, final int clientId) {
 if (TestUtils.isEmpty(mapping) || TestUtils.isEmpty(mapping.get(path)) || TestUtils.isEmpty(mapping.get(path).get(clientId))) {
 logger.logError("mapping must not be empty. full path= ", path, ", clientId= ", clientId, ", Mapping= ",
 mapping);
 return null;
 }
 final int localId = mapping.get(path).get(clientId);
 final String hostname = getHostname(path, localId);
 return hostname;
}

Here mapping is defined as,

private final Map<String, Map<Integer, Integer>> mapping;

Is there anything we can improve in the above code? I do see duplicated stuff as I am checking in the if statement and then using same thing just below it.

• Simon Forsberg: Out of curiousity, what does the TestUtils.isEmpty method do?
• user1950349: They just check emptiness of various datatypes.. String, Collection, Map..

Answer 1

Just don't duplicate it:

Map<Integer, Integer> intermediate = mapping.get(path);
if (intermediate != null) {
 Integer localId = intermediate.get(clientId);
 if (localId != null) {
 return getHostname(path, localId);
 }
}
// log error here
return null;

Alternatively, you could throw the above into a helper function:

private Integer getLocalId(String path, int clientId)
{
 Map<Integer, Integer> intermediate = mapping.get(path);
 if (intermediate != null) {
 return intermediate.get(clientId);
 }
 return null;
}

And then have just the one check:

private String getAddress(final String path, final int clientId) {
 Integer localId = getLocalId(path, clientId);
 if (localId == null) {
 // error
 return null;
 }
 return getHostname(path, localId);
}

• TheCoffeeCup: One of your return nulls don't have a ending semicolon.
• Barry: @TheCoffeeCup You have the rep, you could've added it :)
• TheCoffeeCup: I really don't like editing code on answers or questions; if you think I should, then I will: on yours posts only.
• Barry: @TheCoffeeCup I mean, don't completely rearrange logic or anything. But if it's fixing typos? Yeah, of course, have at it.

Answer 2

With some Java 8 magic, you can use Optional and some repeated map and filter to get your result.

Optional<Integer> localId = Optional.ofNullable(mapping)
 .filter(TestUtils::isNotEmpty)
 .map(m -> m.get(path))
 .filter(TestUtils::isNotEmpty)
 .map(m -> m.get(clientId))
 .filter(TestUtils::isNotEmpty);
if (!localId.isPresent()) {
 logger.logError("mapping must not be empty. full path= ",
 path, ", clientId= ", clientId, ", Mapping= ", mapping);
 return null;
}
final String hostname = getHostname(path, localId.get());
return hostname;

Here, the TestUtils.isNotEmpty(...) method is the negation of your existing TestUtils.isEmpty(...).

• user1950349: I am still on Java 7. Do you think i can still use guava optional here instead of Java 8?
• Simon Forsberg: @user1950349 while you could do that, you wouldn't really gain anything as you couldn't use Java 8's neat lambdas.