4
\$\begingroup\$

I have a Multivalued map (javax.ws.rs.core.MultivaluedMap<String, String>) which I want to convert to regular HashMap so I got below code:

private Map<String, String> convertMultiToRegularMap(MultivaluedMap<String, String> m) {
    Map<String, String> map = new HashMap<String, String>();
    if (m == null) {
        return map;
    }
    for (Entry<String, List<String>> entry : m.entrySet()) {
        String qKey = entry.getKey();
        List<String> values = entry.getValue();
        if (values.size() > 1) {
            String val = "";
            int i = 0;
            for (String s : values) {
                if (i > 0) {
                    val += ",";
                }
                val += s;
                i++;
            }
            map.put(qKey, val);
        } else {
            map.put(qKey, values.get(0));
        }
    }
    return map;
}

Is there any improvement I can do here? I am using Java7.

\$\endgroup\$
7
\$\begingroup\$

The main part of the code is converting the List<String> value into a String delimited by a comma. Instead of using an external i variable, you can use a StringBuilder and append the comma or not depending on whether it is empty or not.

Regarding your current code:

  • Try to avoid concatenating String with +. You should use a StringBuilder when necessary.
  • You don't need to make a separate code path for the case where the list is empty in the map value: the regular path handles it also.
  • You don't need to store the key and the value in local variables.
  • It is indeed a very good idea to return a new empty map instead of null when the incoming map is null.
  • If the value stored in the multimap is an empty list, then your code will throw an exception because it tries to access the element 0 when there is no such element.

This would be a proposed code for Java 7:

private Map<String, String> convertMultiToRegularMap(MultivaluedMap<String, String> m) {
    Map<String, String> map = new HashMap<String, String>();
    if (m == null) {
        return map;
    }
    for (Entry<String, List<String>> entry : m.entrySet()) {
        StringBuilder sb = new StringBuilder();
        for (String s : entry.getValue()) {
            if (sb.length() > 0) {
                sb.append(',');
            }
            sb.append(s);
        }
        map.put(entry.getKey(), sb.toString());
    }
    return map;
}

As a side-note, when you'll upgrade to Java 8, this code can be made simpler by using the new String.join(delimiter, elements) method.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ One case you are missing: If the old stored value was null, then the new stored value will be empty string. You might want to preserve null in that case. Otherwise, good review. \$\endgroup\$ – Pimgd Jan 22 '16 at 21:54
  • \$\begingroup\$ @Pimgd Thanks! Actually, if the old value was null, it would throw a NPE at if (values.size() > 1) so I guess there are no null values. \$\endgroup\$ – Tunaki Jan 22 '16 at 22:07
  • \$\begingroup\$ Would it? I dunno... all I know is that an empty list would throw for values.get(0) with IndexOutOfBoundsException for sure. Depends on how the implementation returns entrySet. \$\endgroup\$ – Pimgd Jan 22 '16 at 22:08
  • \$\begingroup\$ @Pimgd Yes, if entry.getValue(); returns null then the code throws at the if just after. And you're right, if the list is empty, there is an exception, thanks, I'll edit that in. \$\endgroup\$ – Tunaki Jan 22 '16 at 22:10
  • \$\begingroup\$ @Tunaki Thanks for your suggestion. What's the right way to handle point 5? I don't want this code to throw Exception. \$\endgroup\$ – user1950349 Jan 22 '16 at 22:18

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.