Should i prefer readability over safety in my code? [closed]

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;
}