Search This Blog

Tuesday, 21 December 2010

Order matters

Every so often I come across these simple things that after thinking about them I kick myself for not thinking about and knowing sooner. Earlier today was one of those moments!

It might seem natural, assumed, even obvious that for many method calls it doesn't actually matter what way around things go. So take "asd".equals("dsa"); - does it ever make a difference?

In this case, no - but what about if we introduce variables?

String str = "asd";
System.out.println(str.equals("dsa"));
System.out.println("dsa".equals(str));


Again, no difference. Now let's be a bit sneaky however and swap str to null (you may see where this is going!)

String str = null;
System.out.println("dsa".equals(str));
System.out.println(str.equals("dsa"));

Ahah! This time we (somewhat obviously) get a NPE on the third line. But the second line executes without any problems at all (also rather obviously - we're not dereferencing null here!)

Conclusion? When comparing strings, it makes much more sense to adopt a design pattern of putting the literal first rather than a variable name. If this is the case and if the variable happens to be null, then equals will just return false rather than causing an NPE. 99% of time this is the desired behaviour, unless you know your strings should never be null and want to be screamed at if they are. Even so, it'd probably make more sense in this case to put a separate check in rather than relying on the order of the values to give you an NPE - that could easily disappear when someone else comes along and reverses the order without realising it was intentionally the other way around :-)

Saturday, 4 December 2010

The solution

You might expect it to print out 104, "10" being the sum of the fields and then "4" being the number of fields. You would of course be wrong :-)

If you run the following code you won't get a number out at all, you'll get an exception:


Exception in thread "main" java.lang.IllegalArgumentException: Attempt to get test.Outer field "test.Outer$Inner.this$0" with illegal data type conversion to int
at sun.reflect.UnsafeFieldAccessorImpl.newGetIllegalArgumentException(UnsafeFieldAccessorImpl.java:46)
at sun.reflect.UnsafeFieldAccessorImpl.newGetIntIllegalArgumentException(UnsafeFieldAccessorImpl.java:111)
at sun.reflect.UnsafeQualifiedObjectFieldAccessorImpl.getInt(UnsafeQualifiedObjectFieldAccessorImpl.java:41)
at java.lang.reflect.Field.getInt(Field.java:499)
at test.Outer.(Outer.java:21)
at test.Outer.main(Outer.java:29)


The key here is actually down to an implementation detail of Java - specifically how inner classes have a reference to their outer classes. If we make the inner class static (static inner classes don't have a reference to their enclosing class) then the problem goes away and we get the expected output, 104. So what's happening here?

It might help to think how a non-static inner class can always access its outer class. And it also might help if I tell you it's more along the realms of hackery than black magic! The key here is a field, this$0, that the compiler puts in every inner class. This is set by a constructor the compiler also puts in the inner class which is called when an object of this type is instantiated. It's an implementation detail, but quite a geeky and fun one to play around with. If you decompile the class files, you can actually see this extra field and constructor in plain daylight.

I got the idea for this puzzle from http://www.javaspecialists.eu/archive/Issue062.html - this explains this implementation detail far more accurately if you want to take a look.

So, we've got an extra field in the class that's of type Outer, not int. So when we try to call getInt() on this field, we understandably get an error.

As a follow on puzzle which you should now be able to answer, if we change this line:

catch(IllegalAccessException ex) {}

to:
catch(Exception ex) {}


what's printed out now?


The answer? 105. The values that are ints are still summed, so we get 10, but there's still 5 fields in the Inner class!

This is quite frankly a shoddy way to do things anyway, and the chances of doing something like this in real life are rather remote. But how could we fix it? One option is to check for synthetic fields and ignore them - synthetic fields are ones generated by the compiler and not by the user. So if we put if(field.isSynthetic()) continue; as the first line in the for loop and check for similar synthetic fields in counting them at the end, the problem goes away. There's a catch though - there's no requirement for the compiler to actually mark anything as synthetic. It's a bit like using a well established class in the sun package - while it might help in situations like this and is likely to be fine, it shouldn't really be relied upon.

This may seem like a silly example, but it does serve a few lessons. First, as shown by the follow on puzzle, if you just catch all exception types and expect everything to work, you may well get caught out by bugs you weren't expecting, but can really bite you later. Secondly, don't use reflection to loop through fields. It's just plain wrong. If you really have to, at least check for synthetic fields and make sure the fields are of the type you expect. And lastly, decompiling compiled code and looking at the results can be rather interesting in uncovering some Java implementation details!