How not to implement Comparable

I have already blogged about the danger of numeric overflows. I recently came across this example in Java course materials (!!!):

public class Foo implements Comparable<Foo> {
private int number;
public int compareTo(Foo o) {
  if (this == o) return 0;
  return number - o.number;
}
}

How do you think Integer.MAX_VALUE compares to negative numbers? It will appear smaller. This reminds me of even worse case we encountered in a real codebase. Look at this:

public class Foo implements Comparable<Foo> {
private long id;
//...
public int compareTo(Foo o) {
  if (this == o) return 0;
  return (int)(id - o.id);
}
public boolean equals(Object o) {
  return o != null && (o instanceOf Foo) && compareTo((Foo)o) == 0;
}
}

How do you think new Foo(8325671243L) and new Foo(25505540427L) compare? They are equal, but I will do it ala Weiqi Gao and leave you to find out why... :-)

Static initializers - update

After some interesting comments to my previous write-up I decided to make a follow-up post with some additional details.

Here is the code from Effective Java:

public class Person {
 private final Date birthDate;
 //...
 private static final Date BOOM_START;
 private static final Date BOOM_END;

 static {
   Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone("GMT"));
   gmtCal.set(1946, Calendar.JANUARY, 1, 0, 0, 0);
   BOOM_START = gmtCal.getTime();
   gmtCal.set(1965, Calendar.JANUARY, 1, 0, 0, 0);
   BOOM_END = gmtCal.getTime();
 }

 public boolean isBabyBoomer() {
   return birthDate.compareTo(BOOM_START) >= 0 &&
     birthDate.compareTo(BOOM_END) < 0;
 }
}

and here's how I would have changed it:

public class BabyBoom {
 private static final BabyBoom boom = new BabyBoom();
 private Date start = null;
 private Date end = null;

 private BabyBoom() {}

 public static BabyBoom getInstance() {
   if (boom.start == null || boom.end == null) {
     Calendar gmtCal = Calendar.getInstance(TimeZone.getTimeZone("GMT"));
     gmtCal.set(1946, Calendar.JANUARY, 1, 0, 0, 0);
     boom.start = gmtCal.getTime();
     gmtCal.set(1965, Calendar.JANUARY, 1, 0, 0, 0);
     boom.end = gmtCal.getTime();
   }
   return boom;
 }

 public boolean contains(Date birthDate) {
   return birthDate.compareTo(start) >= 0 && birthDate.compareTo(end) < 0;
 }
}

public class Person {
 private final Date birthDate;
 //...
 public boolean isBabyBoomer() {
   BabyBoom boom = BabyBoom.getInstance();
   return boom.contains(birthDate);
 }
}

I didn't synchronize getInstance, because initializing the dates twice does no harm, so it's not worth the price of synchronization. However I did check that both fields are initialized before returning the object in getInstance. 

A case against static initializers

"Effective Java" is an excellent book. I recently bought the 2nd edition, and it is absolutely fabulous, priceless. However after quite some time in the industry, I've learnt not to take any advice blindly. First edition was also excellent, but several of the items were revisited since then. So here's an item that I have mixed feelings about - Avoid creating unnecessary objects. The advice is to use static initializers for the expensive computation. 

Static initializers are double-edged sword. It's like with the stock exchange in times of crisis - for a particular individual it may be a good idea to sell the stock, but the trouble is that everybody's doing it, and in the end everybody's losing big-time. Same applies to static initializers. One or two may seem harmless, but they add up and together create a big problem. The fact that static initializers are (edit) may be invoked at program start-up affects everybody and since they potentially interfere with classloading, it's very hard (edit) harder to debug them if anything goes wrong.

Here is how it usually gets out of hand: people start with initializing static members in static blocks. Map of values, sort of configuration details. That alone sounds harmless. But soon comes the time when the values in a map need to be read from a properties file, so here we got IO within static block. Uh oh, better catch these exceptions. Before you notice the whole thing turns into a puzzler. Don't believe me? Here's a real problem I had.

Server in Java, client is either applet or JNLP. On certain machines, only one of them can run. You run server first - client never comes up, no error whatsoever. You reboot and connect as client to another machine - no problem. But if you try to start up the server locally - silent death. A team in India spends months on it. In vain. The whole release is detained, escalation to senior management. The thing ends up on my desk after a ruthless blame game between teams. Long story short: it's a DirectX problem. Why the **** does the server need DirectX? Ah. What's next after reading defaults from a properties file? Reading them from the database. Oh, but it's a different process, and the database needs to be up. So we not just connect to database from initialization block, we wait for the database process to be up. Great idea. How? We follow "best coding practices" and reuse: find a poller utility somewhere in the JDK. Apparently there is a java.awt.Timer. Why not? Great idea. Apparently, a touch of one AWT class causes a bunch of other AWT classes to load, which in turn loads DirectX and OpenGL dlls. And guess what - Windows on some machines has a nasty bug, that only allows to load them once per machine, regardless of the user. And when another user tries to do it - the loading gets stuck. And our server is of course a system process, while the client belongs to the logged in user. 

Since it was a last minute fix, we solved it with some JVM flags that disabled DirectX and OpenGL. The problem was not the fix, but the diagnosis. If it was part of the regular code, it would have been easy to connect with a debugger, see what call gets stuck, investigate it from there. But as it was part of start-up, people didn't know where to look. Not to mention the man-months accumulatively spent by developers who waited for the server to restart when testing. 

So... what's the lesson here? Life is better without static, avoid it as much as you can. 

Java... tea?

No clone for you!

Java Practices call Cloneable/Object#clone API pathalogical and suggest to avoid it. I have to agree. Here's my little story. 

I have a method that receives a certain object, which is Cloneable. Now I want to actually clone that object, so I am calling the clone method. Makes sense, no? Apparently not. My slightly outdated version of IDEA was confused, just as I was, and did not complain. But Javac was cruel and uncompromizing. No clone for you. 

Because Cloneable does not require a clone() method, and Object#clone() can be protected. WTF? My objects subclass different not-necessarily-cloneable classes, so they can't have a common cloneable superclass. So I'll define my own ReallyCloneable interface that has a public clone method, sounds simple enough...? Not!

public interface ReallyCloneable extends Cloneable {
    public Object clone() _
}

Now I realize there's CloneNotSupportedException to throw. A checked exception. I have 2 options: be a good girl, adhere to best practices and clone the signature of Object#clone, or ... not. Let's explore both options. 

public interface ReallyCloneable extends Cloneable {
    public Object clone() throws CloneNotSupportedException;
}

Now all my classes (let's assume they are simple enough to contain only primitives, Strings and arrays of the above) implement ReallyCloneable and add those 3 lines:

public Object clone() throws CloneNotSupportedException {
    return super.clone();
}

But whoever clones my objects also needs to add:

try {
    //call clone
} catch (CloneNotSupportedException e)  {
    //handle it... how???
}

Because implementing Cloneable and all the rest means that JVM will not necessarity throw CloneNotSupportedException, but as far as Javac is concerned, it still may. So no matter what, the caller needs to prepare himself for the worst. Then why bother with interfaces and all that, I ask? People say Java is verbose. I don't mind verbose, but bloated with boiler-plate to this extent?! 

The other option is not to declare the exception in ReallyCloneable. This is possible because overriding rules do not require to throw the exceptions of the overridden method.

public Object clone() {
  try {
    return super.clone();
  } catch (CloneNotSupportedException e) {
    //hm...? 
  }
}

Even though JLS allows it, my IDE and static analysis tools will complain. So I have to either reconfigure them or add some "escape" annotations. More typing... By now I feel like a typomaniac. And wait, I should handle the exception - I don't want to swallow it. Besides, what if someone really objects to being cloned? There is a need for a runtime exception. 

public class CloneFailedException extends RuntimeException {
public CloneFailedException() {
super();
}
  public CloneFailedException(CloneNotSupportedException e) {
    super(e);
  }
}

and

public Object clone() {
  try {
    return super.clone();
  } catch (CloneNotSupportedException e) {
    throw new CloneFailedException(e); 
  }
}

Now at least the callers don't have to catch. But they will still have to cast, because there is no way to express a self type with Java's static types system. Sigh.

And all this why? and for what? Couldn't we just have Cloneable with a public clone and a RuntimeException to throw if we didn't want to be cloned? Simple as that? 

This is yet another example of static type system's failed attempt to protect us from ourselves.