4
\$\begingroup\$

I had a test for a Java-position where they asked me to manipulate JSON-data from an API.

Once my overall parsing of the data into Maps were done.

The test required me among other things to implement:

  • Parameter for sorting the data (ASC, DESC) in Maps.
  • Parameter for filtering the selection of names in Maps.
  • Parameter for only showing data with odd or even occurrences in Maps.

I passed the test, but did not get the job with the feedback: "How you implemented the sorting and filtering of the maps seemed sloppy, and it seems like you rushed through that part of the test and there's no real code-maintainability in mind for these."

So my question is: How could I have done this implementation in a better way? What flaws do you see?

These maps contains the parsed JSON-data, you will see these being used below.

public static Map<String, Integer> firstNames; /** K= names, V= occurrences of that name **/
public static Map<String, Integer> surNames;

SortMap.java

public class SortMap {

    /**
     * ascending = 1, descending != 1.
     */
    public static <K, V extends Comparable<V>> Map<K, V> sortByValues (final Map<K, V> unsortedMap, int ascending) {
        Map<K,V> sortedMap = new LinkedHashMap<>();
        unsortedMap.entrySet()
                .stream()
                .sorted(Map.Entry.comparingByValue((Comparator<? super V>) (ascending == 1 ? Comparator.naturalOrder() : Comparator.reverseOrder())))
                .forEachOrdered(x -> sortedMap.put(x.getKey(), x.getValue()));
        return sortedMap;
    }

    public static <K extends Comparable<? super K>, V> Map<K, V> sortByKey(Map<K, V> unsortedMap, int ascending) {
        Map<K,V> sortedMap = new LinkedHashMap<>();
        unsortedMap.entrySet()
                .stream()
                .sorted(Map.Entry.comparingByKey((Comparator<? super K>) (ascending == 1 ? Comparator.naturalOrder() : Comparator.reverseOrder())))
                .forEachOrdered(x -> sortedMap.put(x.getKey(), x.getValue()));
        return sortedMap;
    }
}

Usages I provided to solve the tasks:

Map<String, Integer> firstNamesSorted = SortMap.sortByKey(firstNames,-1);
Map<String, Integer> surNamesSorted = SortMap.sortByKey(surNames,1);

FilterMap.java

public class FilterMap {

    public static <K, V> Map<K, V> filterByKey(Map<K, V> map, Predicate<K> predicate) {
        return map.entrySet()
                .stream()
                .filter(entry -> predicate.test(entry.getKey()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    }

    public static <K, V> Map<K, V> filterByValue(Map<K, V> map, Predicate<V> predicate) {
        return map.entrySet()
                .stream()
                .filter(entry -> predicate.test(entry.getValue()))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
    }

}

Usages I provided to solve the tasks:

Map<String, Integer> filterUnevenNumbers = FilterMap.filterByValue(firstNames, value -> value % 2 == 0);
Map<String, Integer> filterNames = FilterMap.filterByKey(firstNames, value -> value.matches("(?i).*as.*"));

My own reflections of my code is:

Sorting:

  • -Could have used an enum instead of an int, but using any modern codeeditor you will see my javaDocs comment and also the ascending variable that you provide input for (removed javaDocs comments to reduce clutter in this instance).
  • +Uses generics.

Filtering:

  • +Uses generics.
  • +Dynamic implementation that can filter basically anything as long as you can create the expression using lambdas.
  • +Can be used for alot more purposes than what they asked for.
\$\endgroup\$
2
  • \$\begingroup\$ You mentioned Javadoc. Could you show that Javadoc please? It would help us understand the job reviewer's comments. \$\endgroup\$
    – markspace
    Jan 15 '20 at 18:53
  • \$\begingroup\$ Its just a comment that elaborates on ASC = 1 and DESC != 1. \$\endgroup\$
    – Joel
    Jan 15 '20 at 20:56
1
\$\begingroup\$

Parameter for sorting the data (ASC, DESC)

If the task actually used 'ASC', 'DESC', then I'd say that there's a good chance they were expecting you to use an enum to make it easy to select the sort type. If not, then personally I'd have preferred a boolean for the ascending parameter. Using magic numbers like '1' to indicate sorting direction is going to be a potential source of mistakes going forward, no matter what your JavaDoc says. It's not intuitive that '-1', '0' and '2' all sort descending whilst '1' sorts ascending.

Filtering

Both of your filter methods do essentially the same thing. The difference being that one filters by key and the other by value. If you want to filter by key + value, then you have to call one filter, then the other, creating temporary maps at each interim step. It seems like the complexity that you're really trying to hide here is the stream + collect steps. Going down this route I'd have been tempted to use a BiPredicate instead and pass both the key + value from each EntrySet to the predicate. This would allow the client to decide what they wanted to filter by (key, value or both). Something like:

private static <K, V>Map<K,V> filter(Map<K,V>map, BiPredicate<K,V> filterPredicate) {
    return map.entrySet()
            .stream()
            .filter(e->filterPredicate.test(e.getKey(), e.getValue()))
            .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));

Naming

It's also worth noting that naming is reasonably important... One of your examples is:

Map<String, Integer> filterNames = FilterMap.filterByKey(firstNames, 
                                                   value -> value.matches("(?i).*as.*"));

So, you're calling filterByKey with a lambda parameter value. This isn't terrible, but since you also have a method filterByValue, I think it can create confusion i.e. did you mean to call filterByKey or filterByValue. If you're using a proper name for the lambda parameter, try to make sure it matches what you're expecting to be passed into the lambda (in this case key). If you are just saying it takes someValue then it's usually better to just use a single letter like x->.

\$\endgroup\$
1
  • \$\begingroup\$ There are some good and valid points in here, and I was not aware of BiPredicate +1. However regards to the value thing, it is the value of that key that I’m filtering, but yeah, x would suffice as well. I’m still confused regarding their critique of my code - I’ve asked for further explanation but have yet to receive it. As this provides some constructive points. I’ll accept it as the answer. Thank you for your time. \$\endgroup\$
    – Joel
    Jan 17 '20 at 16:34
1
\$\begingroup\$

Assuming the requirement was to maintain initial map as immutable object -- just a few concerns re those new statements in sorting. They violate SRP, at least. Either accept already created (mutable) to populate with results, or (I prefer) accept a factory to get one. Same apples to collection step in filtering.

If, however, it is given that input map can be mutated during the process, then whole solution should be written with this in mind and, perhaps, stream is not the right choice here.

\$\endgroup\$
2
  • \$\begingroup\$ No conditions regarding immutability was declared. How is it possible to sort / filter an immutable map? Could you please provide some code-alternatives to illustrate your points? \$\endgroup\$
    – Joel
    Jan 15 '20 at 21:37
  • \$\begingroup\$ Perhaps I didn't make myself clear. There are two choices I see in the definition of the task: (a) use the given Map as a source of elements and return a new instance of Map (maybe even SortedMap) with sorted elements; or (b) sort the content of the give map (i.e. re-arrange it's elements) without producing a new instance. \$\endgroup\$ Jan 16 '20 at 9:51

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.