1
\$\begingroup\$

The idea is to obtain the keys as a list ordered by the natural ordering of the keys but first of them all the ones that have priority, stated by the method hasPriority(). The custom comparator code block feels out of place and I wonder if it is possible to improve the code:

private List<String> orderedKeys() {
    return this.variables.entrySet()
            .stream()
            .sorted((e1, e2) -> {
                if (e1.getValue().hasPriority() && e2.getValue().hasPriority()) return e1.getKey().compareTo(e2.getKey());
                if (e1.getValue().hasPriority()) return -1;
                if (e2.getValue().hasPriority()) return 1;
                return e1.getKey().compareTo(e2.getKey());
            })
            .map(e -> e.getKey())
            .collect(Collectors.toList());
}

Edit: this.variables is a reference to a Map of String Keys and custom objects as values that have the boolean method hasPriority().

\$\endgroup\$
2
  • \$\begingroup\$ Can you post a bit more info, like what variables is? \$\endgroup\$
    – Tunaki
    Jan 24 '17 at 16:16
  • \$\begingroup\$ Done, it is a Map<String, MyObject> with String keys and custom objects as values. \$\endgroup\$
    – lightlazer
    Jan 24 '17 at 16:21
2
\$\begingroup\$

You can use the new utilities in the comparator class. Unfortunately you'll have to help the compiler a bit with the generic type induction.

Assuming, that the value class is "Whatever", this can be done like this:

    map.entrySet().stream()
        .sorted(Comparator.comparing((Map.Entry<String, Whatever> e) -> e.getValue().hasPriority())
                          .thenComparing(Map.Entry::getKey))
        .map(Map.Entry::getKey)
        .collect(Collectors.toList());
\$\endgroup\$
0
\$\begingroup\$

You can write it a bit shorter
Use ^ for xor, so you can shortening up the last return statement
I don't see more greate improvements, because you need both - keys and values - from your map

private List<String> orderedKeys() {
    return this.variables.entrySet()
            .stream()
            .sorted((e1, e2) -> e1.getValue().hasPriority() ^ e2.getValue().hasPriority() ? e1.getKey().compareTo(e2.getKey()) : e1.getValue().hasPriority() ? -1 : 1)
            .map(Map.Entry::getKey)
            .collect(Collectors.toList());
}

Additional you can use Comparator<String> SC = Comparator.naturalOrder()
and then use this string-comparator to compare the two keys, because this safes you against NullPointerException

private static final Comparator<String> SC = Comparator.naturalOrder();
private List<String> orderedKeys() {
    return this.variables.entrySet()
            .stream()
            .sorted((e1, e2) -> e1.getValue().hasPriority() ^ e2.getValue().hasPriority() ? SC.compare(e1.getKey(), e2.getKey()) : e1.getValue().hasPriority() ? -1 : 1)
            .map(Map.Entry::getKey)
            .collect(Collectors.toList());
}
\$\endgroup\$
0
\$\begingroup\$

This is a good example how refactor unfriendly lambda expressions are.

This compact expression hides its semantic intention. You can easily see it: There is no type information for the objects in focus available to identify design flaws. So we can only guess what this expression REALLY means.

I can see you want to hold "Variable" objects in a Map. They were mapped by a key which is a String. So now I guess that this String is considered as the name of the variable. Under this assumption you may have variables with higher priority than others and if you cannot decide the order by priority you go with the lexicographic order.

Maybe your "Variable" objects are missing a name attribute. If so you do not need to traverse the "Entry" objects of the map. Simply use the values. Finally you will end up with following:

public class SortedLamdbaExample {


    public class Variable {

        private String name;
        private boolean priority;

        public Variable(String name, boolean priority) {
            this.name = name;
            this.priority = priority;
        }

        public boolean hasPriority() {
            return priority;
        }

        public String getName() {
            return name;
        }

    }


    public class VariableComparator implements Comparator<Variable> {

        @Override
        public int compare(Variable arg0, Variable arg1) {
            if (arg0.hasPriority() && arg1.hasPriority()) return arg0.getName().compareTo(arg1.getName());
            if (arg0.hasPriority()) return -1;
            if (arg1.hasPriority()) return 1;
            return arg0.getName().compareTo(arg1.getName());
        }

    }


    private Map<String, Variable> variables;



    private List<String> orderedVariableNames() {
        return this.variables.values()
                .stream()
                .sorted(new VariableComparator())
                .map(variable -> variable.getName())
                .collect(Collectors.toList());
    }


}

This implementation may be semantically wrong to your example BUT it does not hide important information.

Doesn't matter if my guess was wrong or not. The missing type information makes it hard for others to really help you. The only artefact I was able to use to identify your intention was the variable name "this.variables". Any other suggestion would be reformulating the statement without knowing about any general design issues.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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