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.
Intro
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):
|
|
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<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 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.
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
1public 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 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.
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.