Compare two map objects and show relative information Java 8

Question

I have 2 maps. First map consists of order in which a specific set of services is executed. Second map contains execution time each service took. I need to display this information. Following is my piece of code. How can this be improved by performance and syntax?

public static String getAllExecTimes(MasterVO masterVO, boolean displayAll) {
 StringBuilder builder = new StringBuilder();
 Map<Integer, DurationVo> durationMap = null;
 Map<Integer, Integer> orderMap = null;
 List<Integer> keys = null;
 if (displayAll) {
 builder.append("\nSection A\n");
 durationMap = masterVO.getMapA();
 orderMap = masterVO.getOrderMap();
 keys = new ArrayList<>(orderMap.keySet());
 keys.sort(new CustomComparator());
 for (Integer execOrder : keys) {
 Integer target = orderMap.get(execOrder);
 if (null != target) {
 builder.append(durationMap.get(target));
 }
 }
 if (null != masterVO.getMapB()) {
 builder.append("\nSection B\n");
 durationMap = masterVO.getMapB();
 keys = new ArrayList<>(durationMap.keySet());
 for (Integer target : keys) {
 builder.append(durationMap.get(target));
 }
 }
 }
 durationMap = masterVO.getMapC();
 keys = new ArrayList<>(durationMap.keySet());
 keys.sort(new CustomComparator());
 builder.append("\nFinal Section\n");
 for (Integer target : keys) {
 builder.append(durationMap.get(target));
 }
 return builder.toString();
}
private static final class CustomComparator implements Comparator<Integer> {
 public int compare(Integer first, Integer second) {
 return first.compareTo(second);
 }
}

• Roland Illig: To prevent downvotes in the future, you should present code that is complete and compilable. Yours is missing the MasterVO and DurationVo classes. It is also missing some test code that demonstrates how getAllExecTimes is used. It is also missing some example data to show what exactly you expect from the code.

Answer 1

Code review in order of appearance:

public static String getAllExecTimes(MasterVO masterVO, boolean displayAll) {

Static methods should be avoided if you may expect them to contain state, such as hints on how to format the class. Why not create an object that performs the formatting?

Boolean parameters are not a good idea. They are pretty unreadable. But in this case it is worse, as the displayAll doesn't give any hint to a user what actually is displayed. What about an enum, e.g. enum Display {DISPLAY_MAP_C_ONLY, DISPLAY_MAPS_A_B_C;}?

Map<Integer, DurationVo> durationMap = null;
Map<Integer, Integer> orderMap = null;
List<Integer> keys = null;

Never ever instantiate variables before they are needed in Java, and don't use the same variable reference for two different maps (A and C). This makes for very error prone code, especially if you are starting to assign null to the variables, which should almost never be necessary. Create separate methods when code is very similar or when variable names are starting to clash.

Why is there a MasterVO and a DurationVo? It's fine to choose all uppercase or camelcase, but please do so consistently.

durationMap = masterVO.getMapA();
orderMap = masterVO.getOrderMap();

You know, a map is a mapping from a set of keys to values. However, you only have duration and order, and it isn't even clear which one is key and what is the value.

keys = new ArrayList<>(orderMap.keySet());
keys.sort(new CustomComparator());

Now this is just not right. If you put the keys into a TreeSet instead then they will be ordered during insertion. Even better, why not use a TreeMap and sort then when you setup the Map?

if (null != target) {

First of all, you should just write this as if (target != null) {; this is a well known anti-pattern for Java, it's just not needed and it hurts readability. It is item #1: "Yoda Conditions" for the "new jargon" article at coding horror.

The bigger question is why you would allow null values in such a map in the first place. Even without knowing the design, I have the feeling that null values should be avoidable in this case, especially since keys without values are simply skipped in the resulting string.

if (null != masterVO.getMapB()) {

Now whole maps seem to be missing in action. Use Optional<...Map> instead.

private static final class CustomComparator implements Comparator<Integer> {
 public int compare(Integer first, Integer second) {
 return first.compareTo(second);
 }
}

Seriously, how does this custom comparator differ from normal integer ordering? Do you need to specify a specific comparator in that case? And why is it called CustomComperator? That name doesn't explain how it is custom.

• Roland Illig: I disagree that all static methods are bad. They are quite useful in many cases. — Regarding the CustomComparator: maybe Tishy Tash just didn't know that Comparator.naturalOrdering() exists.
• Maarten Bodewes: I've not said that all static methods are bad, but I'll explain it better. It will already use that ordering if you don't specify a specific comparator - included that in the explanation too.