Bad Java Example from Book

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


When I visit the library I usually pack random books from which I can hopefully learn something. 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 a singleton is explained):

public class Configuration { private HashMap keyValuePairs; public Configuration() { keyValuePairs = new HashMap(); // load config from file } public HashMap getValues() { return keyValuePairs; } public String getValue(String key) { if (keyValuePairs.containsKey(key)) return keyValuePairs.get(key); else return null; } public void setValue(String key, String value) { if (keyValuePairs.containsKey(key)) keyValuePairs.replace(key, value); else keyValuePairs.put(key, value); writerConfig(); } }

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

// original 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.

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 and the value differed. So at least I learned something useful. But the default implementation is possible doing a map lookup twice, too, so possibly we have 3 unnecessary map lookups compared to the simple method.

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, and getting a value for a key uses the array access [ ] 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 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 and straight-forward. By this you could use Configuration just like any map with all associated advantages, eg you could make it unmodifiable for some usages.


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

public class Configuration { private Map keyValuePairs; public Configuration() { keyValuePairs = new HashMap(); // load config from file } public String getValue(String key) { return keyValuePairs.get(key); } public void setValue(String key, String value) { keyValuePairs.put(key, value); writerConfig(); } }

Seems we lost a bit of weight on the way.

Some Last Words

The book didn’t provide a contact address 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.


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.