Should i prefer readability over safety in my code?

Question

I am dealing with some code that has consistent pattern of creating Map instances like this:

Map<String, String> valuesMap = new HashMap<String, String>();
valuesMap.put("UserName", "Value1");
valuesMap.put("FirstName", "Value2");
valuesMap.put("LastName", "Value3");
valuesMap.put("Email", "Value4");

I thought it can be made Readable like this:

Map<String, String> valuesMap = createMap(
    "UserName", "Value1",
    "FirstName", "Value2",
    "LastName", "Value3",
    "Email", "Value4"
);            

With the help of following method:

private Map<String, String> createMap(String... parameters) {
    Map<String, String> valueMap = new HashMap<String, String>();
    if (parameters.length % 2 == 0) {
        for (int i = 0; i < parameters.length; i+=2){
            String key = parameters[i];
            String value = parameters[i + 1];
            valueMap.put(key, value);
        }
    } else {
        throw new IllegalArgumentException("The parameters to this method must be even in number");
    }
    return valueMap;
}

But by doing this i am losing ability of catching errors at compile time. For example: Someone can easily do the following

Map<String, String> valuesMap = createMap(
    "UserName", "Value1",
    "FirstName", "Value2",
    "LastName", // missing value for key
    "Email", "Value4"
);            

I am tempted to use more readable method, need suggestions from you guys.

Edit:

• There are many instances declared like the first example.
• Keys and values are not populated but are String decalration more like static intializers.
• Its more like a code containing multiple declarations
• Keys and Values are different on every declareation

---- @Jon Skeet Thanks for pointing out the bug.

Answer 1

Despite the various comments, I've seen helper methods like this used quite a bit, for static initialization, and usually found them simpler than alternatives. Of course, it only works when your key and value type are the same - otherwise you could provide overloads for (say) up to 4 keys and 4 values, at which point you get compile-time safety too.

You're not actually skipping safety - you're deferring compile-time safety to execution-time safety. So long as you have at least one unit test which executes that code, you'll find the error then (as you throw the exception). In my experience, maps like this are usually used either in tests, or as static final maps which are never mutated. (For the latter, consider using an immutable map to enforce the lack of mutation...)

That said, I'd refactor the method to just validate the count first:

if (parameters.length % 2 != 0) {
    throw new IllegalArgumentException("Odd number of keys and values provided");
}
// Now we can just proceed
Map<String, String> valueMap = new HashMap<String, String>();
for (int i = 0; i < parameters.length; i+=2) {
    String key = parameters[i];
    String value = parameters[i + 1];
    valueMap.put(key, value);
}
return valueMap;

Note that you had a bug in your population code before, due to the bounds check of your loop.

Answer 2

I urge you to consider doing it this way:

@SafeVarargs
public static <K,V> HashMap<K,V> hashMap(Map.Entry<K,V>... kvs) {
  return map(HashMap::new, kvs);
}

@SafeVarargs
public static <M extends Map<K, V>, K, V> M map(
    Supplier<? extends M> supplier, Map.Entry<K, V>... kvs)
{
  M m = supplier.get();
  for (Map.Entry<K,V> kv : kvs) m.put(kv.getKey(), kv.getValue());
  return m;
}

public static <K,V> Map.Entry<K,V> kv(K k, V v) { 
  return new SimpleImmutableEntry<K, V>(k,v); 
}

Your client code would look like this:

Map<Integer, String> map = hashMap(kv(1, "a"), kv(2, "b"));

and it would be 100% typesafe because the key-value pairs are represented at the static type level.

BTW this doesn't fundamentally depend on Java 8 features. Here's a simplified version for Java 7:

@SafeVarargs
public static <K,V> HashMap<K,V> hashMap(Map.Entry<K,V>... kvs) {
  HashMap<K, V> m = new HashMap<>();
  for (Map.Entry<K, V> kv : kvs) m.put(kv.getKey(), kv.getValue());
  return m;
}

Answer 3

A method that takes a long list of parameters of the same type is considered bad practice.

In your case rather than using a Map that contains the details for your user. Consider creating a Class User with the members userName, firstName, lastName and email.

Answer 4

If you think about it, readability and safety are highly correlated. Extremely readable code is so transparent that any malformation shines on the background of the simplicity of the expression of your idea. Extremely safe code has to be readable, because by making code reviews difficult you end up with mistakes done during subsequent maintenance.

There's no conflict to balance here. Anything that you will do to make the helper method's implementation safer, will make its uses more readable.

For example, it doesn't have to take strings, it can take pairs of strings (helper class). Or it can check the sanity of its input. Or you can use the put method as shown in your question. Any of these alternatives makes the result safer and more readable.

"Readable" means that you don't have to scrutinize the pattern every time you see it, right?

Answer 5

This is a good idea--great actually, but you do have to be careful of errors. In your case, you need to complain really loudly if there are an odd number of entries. I often do this by preventing program startup where possible unless the data is externalized at which point you just want to complain loudly and possibly revert to previous values so as to not break a production server with bad data.

If you can identify a name vs a "value" you should do that as well.

I do this kind of thing quite a bit--just remember that whenever you remove compile time error checking with code like this you are assuming runtime responsibility for it!

By the way, you could also improve both error checking and readability further (The comma used is just a suggestion, you just pick some separator that cannot go in either field, comma, colon and tildie are good choices):

String[] values=new String[]{
    "Name1,Value1",
    "Name2,Value2",
    ...
}

Then to parse:

for(String s:values) {
    String[] splitted=s.split(",")
    assert(splitted.size == 2)
    valuesMap.put(splitted[0],splitted[1])
}

I use this pattern with the string array initialization constantly. You can include such things as method names to bind your data to code reflectively.

Side-advantages of doing this is that it is really easy to externalize making runtime configuration and externalization possible with almost no additional work.