Recently I encountered a blog entry describing a “beautiful” way of making the code more readable by creating a gereric helper method to perform object casting. Today I want to share some opinions about this improvement and general thoughts about coding.

The premise

In essence author of this post was trying to solve following problem: imagine you have a context object (HttpSession for example) that is used as a map of String keys to some possibly large, complicated generic values of different types. The contents of it are set by an external code out of your control. You can request any value from this context by specifying the key, but the interface contract is to return an Object so if you want to use it you need to cast it into proper type. In that case your code usually looks quite ugly:

1:
2:
3:
4:
5:
    HttpSession session = getHttpSession()
    // We know that an attribute "phonebook" is a map
    // of names (String) to sets of telephone numbers (String)
    Map<String, Set<String>> phonebook =
            (Map<String, Set<String>>) session.getAttribute("phonebook");

The beautiful solution to that problem according to the author was creating following helper method:

1:
2:
3:
    public static <T> T cast(Object obj) {
        return (T) obj;
    }

Now all you need to do is to import it in a static way in every class and replace the ugly casting with a short and readable invocation of our beautiful method:

1:
2:
3:
4:
5:
    HttpSession session = getHttpSession()
    // We do not need to know anything about the type
    // of "phonebook" - cast() method does everything for us
    Map<String, Set<String>> phonebook =
            cast(session.getAttribute("phonebook"));

So what’s wrong?

It may seem that this is a huge improvement for readability as before almost half of the code was casting, which obscured the real logic of the program. But this helper method in fact is no help at all, as it creates serious vulnerabilities to error. In fact it basically removes type-checking from your code and therefore makes every tiny error to appear in runtime instead of compile time.

The first problem you may run into is when by mistake you use incompatible types for casting. The chance of that happening is very high: it may happen just by copy-pasting wrong part of code, during refactoring when you change the type of object in the context or just because of naming confusion. The problem is not that error will happen but that you will find out about it only after running the program. See the following errors – all resulting in ClassCastException:

1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
   // Naming issue: some may think telephone number is actually a number
    Number telephoneNumber = cast("1234567");

    // Refactoring: type of "digits" changed from List<Character>
    // to Set<Character> but not all places using it have been updated:
    session.setAttribute("digits", setOfDigits);
    List<Character> characters = cast(session.getAttribute("digits"));

    // Copy-paste error: this line was copy-pasted from other
    // place in the code and the old key was left by mistake
    String authorName = cast(session.getAttribute("authorId"));

The errors shown above are serious, but not the worst thing that can happen. Since there is no way for the compiler to find them it is very possible that they will appear during code execution, possibly in production crashing your application. The good thing about them is that when they’ll appear you will get a stack trace pinpointing the location of an error. The following code shows a worse kind of error – the one that will kill your program and leave you no clue why that happen:

1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
   // we have a map of Integers to Integers
    Map<Integer, Integer> integerMap = new HashMap<Integer, Integer>();
    integerMap.put(123, 123);
    session.setAttribute("map", integerMap);

    // Because of an error we cast it to a map of String to String
    Map<String, String> badMap = cast(session.getAttribute("map"));

    // we pass the map between many objects and doing many operations on it
    String s1 = badMap.get(1);
    badMap.put("2", "2");
    String s2 = badMap.get("2");
    String s3 = badMap.get(2);
    String s4 = badMap.get(123);

It is obvious the mistake was made, but when it will manifest itself? Unfortunately this will not happen in line 7 when casting is done. Even the code in lines 10-13 will not result in a exception! Due to many factors (see posts about map type safety and checked set) the error will wait until line 14 to crash your application. The stack trace you will get probably won’t help you much as line 14 in reality may be far away from the place when casting is done.

Is there a way to make it better?

The biggest problem with the cast() method is not the casting itself, but the uncontrolled way it is being done and the vulnerability to simple code editing errors. You might say: “The cast() method is far from perfect, but still most of the errors above apply to the normal casting. So maybe it’s worth to use it to gain readability?” No, it’s not. There is a better solution for the problem of dealing with context-like objects: writing an adapter for it that will provide a readable and type safe interface. See the following snippet:

1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
   public class HttpSessionAdapter {
        private final HttpSession session;

        public HttpSessionAdapter(HttpSession session) {
            this.session = session;
        }

        public Map<String, Set<String>> getPhonebook() {
            return (Map<String, Set<String>>)
                    session.getAttribute("phonebook");
        }
    }

This adapter provides type-checking and allows to encapsulate all vulnerabilities in one place, therefore drastically decreasing the chance that anyone using the session’s attributes will make an runtime error. What is more it also gives a boost to readability of the code – now to get a phonebook no casting or attribute requests are needed, you just ask the adapter for it!

1:
2:
3:
4:
5:
6:
7:
    HttpSession session = getHttpSession()
    HttpSessionAdapter sessionAdapter = new HttpSessionAdapter(session);

    // You do not need to know now the phonebook type, as
    // Java typing will enforce it. Also you do not need to know
    // the structure of session nor the names of the keys!
    Map<String, Set<String>> phonebook = sessionAdapter.getPhonebook();

It is important to state that this solution is not bulletproof, but thats because of the way we stated the problem. The fact that we do not control the types of objects originally inserted into the session can cause an runtime exception if an error in inserting occurs. To remove that vulnerability we would have to add ’set’ methods to control the types of inserted object the same way we control it in ‘get’ methods.

Conclusion

You may be tempted in solving your problems by using the features of Java in a non standard way, trying to find magical solutions that show your deep knowledge of the language. Unfortunately this very often leads to design decisions that are more ‘cool’ than thought trough. Whenever possible try to obey the OOD rules as they usually are the most reliable and error/change proof of coding, even if at the time you do not see the direct benefits from that.