4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Out of curiousity, what does the TestUtils.isEmpty method do? \$\endgroup\$ Dec 18, 2015 at 21:23
  • \$\begingroup\$ They just check emptiness of various datatypes.. String, Collection, Map.. \$\endgroup\$ Dec 19, 2015 at 0:53

2 Answers 2

7
\$\begingroup\$

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);
}
\$\endgroup\$
4
  • \$\begingroup\$ One of your return nulls don't have a ending semicolon. \$\endgroup\$ Dec 17, 2015 at 23:00
  • \$\begingroup\$ @TheCoffeeCup You have the rep, you could've added it :) \$\endgroup\$
    – Barry
    Dec 17, 2015 at 23:01
  • \$\begingroup\$ I really don't like editing code on answers or questions; if you think I should, then I will: on yours posts only. \$\endgroup\$ Dec 17, 2015 at 23:02
  • \$\begingroup\$ @TheCoffeeCup I mean, don't completely rearrange logic or anything. But if it's fixing typos? Yeah, of course, have at it. \$\endgroup\$
    – Barry
    Dec 17, 2015 at 23:03
2
\$\begingroup\$

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(...).

\$\endgroup\$
2
  • \$\begingroup\$ I am still on Java 7. Do you think i can still use guava optional here instead of Java 8? \$\endgroup\$ Dec 19, 2015 at 0:53
  • \$\begingroup\$ @user1950349 while you could do that, you wouldn't really gain anything as you couldn't use Java 8's neat lambdas. \$\endgroup\$ Dec 19, 2015 at 10:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.