Bad Java Example from Book

Today I started reading a book about old and new Design Patterns which I brought from my last visit to the local library, and found some bad piece of Java code.

Intro

I usually pack random books from which I can hopefully learn something when I visit the library. This week I lent Matthias Geirhos: “Entwurfsmuster”, 2015 (the title is German for “Design Patterns”) which promises to explain 37 design patterns new and old. I already own the good old GoF book, but Matthias’ book promised to explain a few more patterns on its cover, and it’s generally not a bad idea to brush up older knowledge.

Bad Java Code

The book’s examples are written in Java, and it states that it is a good source for beginners. But I was quite disappointed (no: it even hurt my eyes) when I found the following piece of code used to explain the Singleton pattern on page 67 (I took the freedom to translate the code to English, otherwise it’s verbatim, and the code is before its conversion to singleton is explained):

 1 public class Configuration
 2 {
 3   private HashMap<String, String> keyValuePairs;
 4   
 5   public Configuration()   
 6   {
 7     keyValuePairs = new HashMap<String, String>();
 8     // load config from file
 9   }
10   
11   public HashMap<String, String> getValues()
12   {
13     return keyValuePairs;
14   }
15   
16   public String getValue(String key)
17   {
18     if (keyValuePairs.containsKey(key))
19       return keyValuePairs.get(key);
20     else 
21       return null;
22   }
23   
24   public void setValue(String key, String value)
25   {
26     if (keyValuePairs.containsKey(key))
27       keyValuePairs.replace(key, value);
28     else
29       keyValuePairs.put(key, value);
30     writerConfig();
31   }
32 }

So what’s wrong with this code?

Incompetent Usage of Java

This is what made me shudder: both methods getValue() and setValue() are unnecessarily complex. Instead of

   // orignal code
   public String getValue(String key)
   {
    if (keyValuePairs.containsKey(key))
      return keyValuePairs.get(key);
    else 
      return null;
   }

a simple

   // improved code
   public String getValue(String key)
   {
     return keyValuePairs.get(key);
   } 

would already do exactly the same, is cheaper on programming (the key is looked up twice in the original code), and is much easier to understand.

As said the same is true for

  // original code
  public void setValue(String key, String value)
  {
    if (keyValuePairs.containsKey(key))
      keyValuePairs.replace(key, value);
    else
      keyValuePairs.put(key, value);
    writerConfig();
  }

which could be written

  // improved code
  public void setValue(String key, String value)
  {
    keyValuePairs.put(key, value);
    writeConfig();
  }

Indeed I didn’t even know that there is a replace() method for maps. But that was introduced to the Map interface in Java 7 as a default method and, in difference to put(), sets the new value only if the key is already contained in the map. At least I learned something useful.

Every slightly experienced Java programmer should know to do the above in the way I propose, and at first glance it is quite surprising that the author of a book on such quite advanced topic does not.

But the explanation for that is simple knowing the following: usually Matthias is programming in C#, and with this knowledge the above becomes perfectly clear. The C# class which is the equivalent of a HashMap is called Dictionary.aspx), and getting a value for a key uses the [ ] operator which would make line 19 from the complete code example look like

    return keyValuePairs[key];   // C# code

But there is a major difference. The C# code throws an exception when the key is not yet contained in the map. So in C# the Java code would make more sense, especially because the a more C#ish code which would be used in an equivalent GetValue() method would use something which cannot be directly ported to Java:

    // C# code:
    public string GetValue(string key) {
      string value;
      keyValuePairs.TryGetValue(key, out value); // returns bool indicating whether key was found
      return value;
    }

It’s quite philosophical to decide which language’s way is better. By providing the operator access C# makes accessing a Dictionary (HashMap in Java) look the same as accessing an array. If you access an array with an undefined index you’ll get an exception in both languages. But as I’m used to Java to my feeling throwing an exception is a bit harsh for a non-existing key, and the addition of the TryGetValue()method seems to prove this point. It’s there because otherwise you’ll need to evaluate the key twice for just getting a value regardless whether it exists. The similarity between arrays with integer keys and maps/dictionaries with any keys is a bit shaky in this regard, because for an array it’s easy to tell which indexes will fail.

Matthias says in the prefix of the book that he uses Java for the example code as it is more widespread than C# which he usually uses. He has indeed written C# books, but my guess is that he has not done much Java programming.

Buggy Code

Even an example should basically do its work. The above example is buggy, because writing the configuration (call to not implemented method writeConfig() in line 30) is not always happening when the configuration is changed. The getValues() method starting in lines 11-14 returns the internal map. Users could modify this map and the Configuration class wouldn’t know of that, and therefore writeConfig() wouldn’t be called although the configuration was changed. That the method returns a HashMap and not a Map is bad programming, too: why forward this implementation detail to the outside world?

Being a code example my preferable solution would be to just remove this method. It is not necessary for understanding the later singleton.

If in the real world returning the map is considered useful one might tend to just make the returned map unmodifiable like

  public Map<String, String> getValues()  // note the changed return value
  {
    return Collections.unmodifiableMap(keyValuePairs);
  }

But if a map access to the key-value pairs is really required a much better idea would be to make Configuration implement Map. This would require to implement various methods, but a lot of them would just forward to the wrapped map. Only the ones which change the map would also call writeConfig(), so it’s a bit of work but perfectly simple. By this you could use Configuration just like any map with all associated advantages, eg you could make it unmodifiable for some usages.

Nitpicking

I’d also make line 3 use Map instead of HashMap. This would change nothing else, but allow to exchange the map implementation, eg by a TreeMap which would allow ordered iteration. Or you could use a thread-safe map implementation instead, which would be quite a good idea for a singleton.

Improved Code

 1 public class Configuration
 2 {
 3   private Map<String, String> keyValuePairs;
 4   
 5   public Configuration()   
 6   {
 7     keyValuePairs = new HashMap<String, String>();
 8     // load config from file
 9   }
10   
11   public String getValue(String key)
12   {
13     return keyValuePairs.get(key);
14   }
15   
16   public void setValue(String key, String value)
17   {
18     keyValuePairs.put(key, value);
19     writerConfig();
20   }
21 }

Seems we lost a bit of weight on the way.

Some Last Words

The book didn’t provide a contact email for Matthias, or I’d written him some of the above directly instead of ranting here behind his back. Sorry, Matthias! I also program in Java and C#, but I’m more experienced in Java. So I’m positive that you’d find similar issues in my C# code.

Porting

Although porting between programming languages is easier than translating between human languages doing it well is not as easy as it might seem. I had to port a class hierarchy from Java to C# recently, and it took me twice as long as I expected (usually I’m much better at estimating). Although both languages are indeed quite similar, some things are quite tedious to port. Most time it took me to port my beloved enhanced Java enums because in C# there is nothing equivalent, it just uses stupid integer values for enums, and all the logic had to go elsewhere. On the other hand this makes using bit flag enums very easy in C# (just add an annotation to your enum), while it is awfully complex in Java.