Peter is a DZone MVB and is not an employee of DZone and has posted 156 posts at DZone. You can read more from them at their website. View Full User Profile

Java Memes Which Refuse to Die

08.23.2012
| 22084 views |
  • submit to reddit

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 interest 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

  1. 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)
  2. Similarly, you might believe that DataInputStream does something useful here when it doesn't
  3. 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 it is much simpler.
Published at DZone with permission of Peter Lawrey, author and DZone MVB. (source)

(Note: Opinions expressed in this article and its replies are the opinions of their respective authors and not those of DZone, Inc.)

Tags:

Comments

Jilles Van Gurp replied on Sun, 2012/08/26 - 10:43am

They fixed the locking issues in Java 1.5 indeed. Anything before that is defacto not supported (with e.g. bug fixes and security updates) at this point and you use it at your own risk. I'd say the combination of synchronization heavy code and an obsolete JVM is a problem waiting to happen. The only reason I could imagine considering this as a viable deployment option at all is a combination of overpriced/obsolete crapware and expensive support/extortion contracts for probably misguided enterprise solutions. But sometimes live sucks that much indeed.

Using singletons (aka global variables) is a bad idea, usually. Use dependency injection instead and let the container worry about creating one or more instances (default is one) and simply inject what you need where you need it. Double locking can be useful for (lazy) initialization logic in places where you don't have a proper dependency injecting container that could offer you something like the spring InitializingBean interface combined with a default scope of singleton, which would normally give you everything you need.

I have on occasions used double locking. Not a big deal, once you understand how it works and why you need it.  Usually a combination of poorly designed and hence poorly testable legacy software and a need to just reliable initialize something once and only once are the reasons. It's good to be a aware that you are adding to the problem though by not refactoring things to be properly testable (i.e. dependency injected).

The reason I prefer double locking here is that making constructors do work other than setting properties to injected values is another anti pattern. So,neither the inner class nor the enum are very nice solutions. At best you're hacking the class loader mechanism to do stuff you shouldn't be doing anyway. 

Then, the StringBuilder vs StringBuffer discussion is pretty much redundant at this point. Hotspot takes care of the redundant StringBuffer synchronization (really: try it with some non trivial example). Also using + on strings in simple expressions gets optimized as well. If you find youself doing a lot of string concatenization, consider using proper marshalling and unmarshalling frameworks for json, xml, protobuf or whatever flavor of structured data format you are handling. It's both faster, less tedious, less error prone, and easier to test.

As for DataInputStream, this is a somewhat useful class but indeed not in combination with a wrapping BufferedInputStream, which would hide most of the API that DataInputStream provides (except for readLine). Doing it the other way around can be useful though if you want to read java primitives and utf-8 strings from a stream. I've personally never used this and you probably shouldn't use this either other than with legacy formats that were produced with DataOutputStream. Otherwise: use protobuf or something similar.  

Peter Lawrey replied on Fri, 2012/12/21 - 3:05pm in response to: Jilles Van Gurp

 You raise allot of interesting points which have validity and I would like to discuss in some detail.  In terms of whether synchronization can be optimised away, I haven't seen a nontrivial case where this is true.

http://vanillajava.blogspot.co.uk/2012/12/can-synchronization-be-optimisede-away.html

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.