Java memes which refuse to die

Also titled; My pet hates in Java coding.

There are a number of Java memes which annoy me, partly because they were always a bad idea, but mostly because people still keep picking them up years after there is better alternatives.

Using StringBuffer instead of StringBuilder

The Javadoc for StringBuffer from 2004 states
As of release JDK 5, this class has been supplemented with an equivalent class designed for use by a single thread, StringBuilder. The StringBuilder class should generally be used in preference to this one, as it supports all of the same operations but it is faster, as it performs no synchronization.
Not only is StringBuilder a better choice, the occasions where you could have used a synchronized StringBuffer are so rare, its unlike it was ever a good idea.
Say you had the code
// run in two threads
sb.append(key).append("=").append(value).append(", ");

Each append is thread safe, but the lock could be release at any point meaning you could get
key1=value1, key2=value2, 
key1=key2value1=, value2, 
key1key2==value1value2, , 
What makes it worse is that the JIT and JVM will attempt to hold onto the lock between calls in the interests of efficiency. This means you can have code which passes all your tests and works in production for years, but then very rarely breaks, possibly due to upgrading your JVM.

Using DataInputStream to read text

Another common meme is using DataInputStream when reading text in the following template (three lines with the two readers on the same line) I suspect there is one original code which gets copied around.
FileInputStream fstream = new FileInputStream("filename.txt");  
DataInputStream in = new DataInputStream(fstream);  
BufferedReader br = new BufferedReader(new InputStreamReader(in));  
This is bad for three reasons
  • You might be tempted to use in to read binary which won't work due to the buffered nature of BufferedReader. (I have seen this tried)
  • Similarly, you might believe that DataInputStream does something useful here when it doesn't
  • There is a much shorter way which is correct.
BufferedReader br = new BufferedReader(new FileReader("filename.txt")); 
// or with Java 7.
try (BufferedReader br = new BufferedReader(new FileReader("filename.txt")) {
    // use br
}

Using Double Checked Locking to create a Singleton

When Double checked locking was first used it was a bad idea because the JVM didn't support this operation safely.
// Singleton with double-checked locking:
public class Singleton {
    private volatile static Singleton instance;

    private Singleton() { }

    public static Singleton getInstance() {
        if (instance == null) {
            synchronized (Singleton.class) {
                if (instance == null) {
                    instance = new Singleton();
                }
            }
        }
        return instance;
    }
}

The problem was that until Java 5.0, this usually worked but wasn't guaranteed in the memory model. There was a simpler option which was safe and didn't require explicit locking.
// suggested by Bill Pugh
public class Singleton {
    // Private constructor prevents instantiation from other classes
    private Singleton() { }

    /**
     * SingletonHolder is loaded on the first execution of Singleton.getInstance()
     * or the first access to SingletonHolder.INSTANCE, not before.
     */
    private static class SingletonHolder {
        public static final Singleton INSTANCE = new Singleton();
    }

    public static Singleton getInstance() {
        return SingletonHolder.INSTANCE;
    }
}

This was still verbose, but it worked and didn't require an explicit lock so it could be faster.

In Java 5.0, when they fixed the memory model to handle double locking safely, they also introduced enums which gave you a much simpler solution.
In the second edition of his book Effective Java, Joshua Bloch claims that "a single-element enum type is the best way to implement a singleton"
With an enum, the code looks like this.
public enum Singleton {
    INSTANCE;
}
This is lazy loaded, thread safe, without explicit locks and much simpler.




Comments

  1. In some cases static blocks also make for a good singleton initialization technique. Granted that this is executed when the classloader ... loads your class, which is not always what you want in a container for example.

    private static instance;

    static {
    instance = new Instance();
    }

    ReplyDelete
  2. @Cosmin In that case I would make the instance final and the constructor private if possible. In the simplest cases you can initialise in one line without a static block.

    ReplyDelete
  3. Yes, fair point, for those cases you can initialize it when declaring the static instance field.

    Funny enough this is *not* what enum does, which instead uses the static block initializer. Probably because it also has to initialize the values array.

    ReplyDelete
  4. Nice collection. I would add the Executor framework to this list. I still find people creating thread pools the old way.

    ReplyDelete
  5. Good stuff. One minor complaint -- code for creating FileReader still has a flag, as it does not pass in encoding, and ends up using the platform default encoding, whatever that is. Which may or may not be what user wants. I know adding it (via FileInputStream) adds one more line, but this (lack of passing encoding) actually belongs as a separate entry on your list.

    ReplyDelete
  6. I never use FileRead because it forces you to use platform default encoding, which usually is a bug waiting to happen.

    The enum as singleton pattern is nice but it forbids you to use have a superclass which makes code reuse bad for some patterns. This will be solved with defender methods in Java 8 but until then I can't use the enum singleton for things like Hamcrest Matchers (which you're supposed to extend from BaseMatcher instead of implementing Matcher directly).

    ReplyDelete
  7. I would take full advantage of JavaSE 1.7 capabilities, as follows:

    try(BufferedReader reader= Files.newBufferedReader(Paths.get("filename.txt"),Charset.forName("UTF-8"))){
    // use reader
    };

    ReplyDelete
  8. Good points. By the way I always prefer + operator instead of StringBuffer or Builider because these are more readable and automatically converted to builder's call. Points on Enum are simply best, share a few more thoughts on Why Enum Singleton are better in Java.

    ReplyDelete
  9. Watch out (java 7): If the BufferedReader constructor throws an exception then the FileInputStream won't be auto-closed. Granted it's not likely that the BufferedReader's constructor would fail, but still... Solution is to assign the FileInputStream to a variable which is then used as input to the BufferedReader constructor. If the FileInputStream constructor succeeds followed by an exception thrown by the BufferedReader constructor, the FileInputStream is correctly closed.

    ReplyDelete
  10. I though Bloch admitted later that it was a bad idea to implement the singleton through Enum. Mostly because the singleton usually has a state, but Enum shouldn't have it. At least it won't be serialized and deserialized properly in this case. Also it looks like as Enum misuse.

    ReplyDelete
  11. @Artem That is a good point. You can't use a serializable state in an enum.

    Using DI, I try not to have stateful singletons (instead have stateful object for which there happens to be one instance), and only really use Singleton for instances which implement an interface but otherwise don't have any state.

    ReplyDelete
  12. @Carlos
    I just today noticed that since Java 7, instead of Charset.forName("UTF-8"), you can use a proper constant: StandardCharsets.UTF_8

    Some other things classes that I would prefer to die away, but still see sometimes being used (probably because lots of old material for learning programming uses them), are the old collection classes from Java 1.0: Vector, Hashtable, Stack.

    Also the Date and Calendar classes should be banned. Everybody should use Joda-Time instead.

    ReplyDelete

Post a Comment

Popular posts from this blog

Low Latency Microservices, A Retrospective

Unusual Java: StackTrace Extends Throwable

System wide unique nanosecond timestamps