Don’t be “Clever”: The Double Curly Braces Anti Pattern


From time to time, I find someone using the double curly braces anti pattern (also called double brace initialisation) in the wild. This time on Stack Overflow:

Map source = new HashMap(){{
    put("firstName", "John");
    put("lastName", "Smith");
    put("organizations", new HashMap(){{
        put("0", new HashMap(){{
            put("id", "1234");
        }});
        put("abc", new HashMap(){{
            put("id", "5678");
        }});
    }});
}};

In case you do not understand the syntax, it’s actually easy. There are two elements:

  1. We’re creating anonymous classes that extend HashMap by writing
    new HashMap() {
    }
    
  2. In that anonymous class, we’re using an instance initialiser to initialise the new anonymous HashMap subtype instance by writing things like:

    {
        put("id", "1234");
    }
    

    Essentially, these initialisers are just constructor code.

So, why is this called the Double Curly Braces Anti Pattern

58731480

There are really three reasons for this to be an anti pattern:

1. Readability

This is the least important reason, it’s readability. While it may be a bit easier to write, and feel a bit more like the equivalent data structure initialisation in JSON:

{
  "firstName"     : "John"
, "lastName"      : "Smith"
, "organizations" : 
  {
    "0"   : { "id", "1234" }
  , "abc" : { "id", "5678" }
  }
}

And yes. It would be really awesome if Java had collection literals for List and Map types. Using double curly braces to emulate that is quirky and doesn’t feel quite right, syntactically.

But let’s leave the area where we discuss taste and curly braces (we’ve done that before), because:

2. One type per instance

We’re really creating one type per double brace initialisation! Every time we create a new map this way, we’re also implicitly creating a new non-reusable class just for that one simple instance of a HashMap. If you’re doing this once, that might be fine. If you put this sort of code all over a huge application, you will put some unnecessary burden on your ClassLoader, which keeps references to all these class objects on your heap. Don’t believe it? Compile the above code and check out the compiler output. It will look like this:

Test$1$1$1.class
Test$1$1$2.class
Test$1$1.class
Test$1.class
Test.class

Where the Test.class is the only reasonable class here, the enclosing class.

But that’s still not the most important issue.

3. Memory leak!

The really most important issue is the problem that all anonymous classes have. They contain a reference to their enclosing instance, and that is really a killer. Let’s imagine, you put your clever HashMap initialisation into an EJB or whatever really heavy object with a well-managed lifecycle like this:

public class ReallyHeavyObject {

    // Just to illustrate...
    private int[] tonsOfValues;
    private Resource[] tonsOfResources;

    // This method almost does nothing
    public void quickHarmlessMethod() {
        Map source = new HashMap(){{
            put("firstName", "John");
            put("lastName", "Smith");
            put("organizations", new HashMap(){{
                put("0", new HashMap(){{
                    put("id", "1234");
                }});
                put("abc", new HashMap(){{
                    put("id", "5678");
                }});
            }});
        }};
        
        // Some more code here
    }
}

So this ReallyHeavyObject has tons of resources that need to be cleaned up correctly as soon as they’re garbage collected, or whatever. But that doesn’t matter for you when you’re calling the quickHarmlessMethod(), which executes in no time.

Fine.

Let’s imagine some other developer, who refactors that method to return your map, or even parts of your map:

    public Map quickHarmlessMethod() {
        Map source = new HashMap(){{
            put("firstName", "John");
            put("lastName", "Smith");
            put("organizations", new HashMap(){{
                put("0", new HashMap(){{
                    put("id", "1234");
                }});
                put("abc", new HashMap(){{
                    put("id", "5678");
                }});
            }});
        }};
        
        return source;
    }

Now you’re in big big trouble! You have now inadvertently exposed all the state from ReallyHeavyObject to the outside, because each of those inner classes holds a reference to the enclosing instance, which is the ReallyHeavyObject instance. Don’t believe it? Let’s run this program:

public static void main(String[] args) throws Exception {
    Map map = new ReallyHeavyObject().quickHarmlessMethod();
    Field field = map.getClass().getDeclaredField("this$0");
    field.setAccessible(true);
    System.out.println(field.get(map).getClass());
}

This program returns

class ReallyHeavyObject

Yes, indeed! If you still don’t believe it, you can use a debugger to introspect the returned map:

debug-output

You will see the enclosing instance reference right there in your anonymous HashMap subtype. And all the nested anonymous HashMap subtypes also hold such a reference.

So, please, never use this anti pattern

You might say that one way to circumvent all that hassle from issue 3 is to make the quickHarmlessMethod() a static method to prevent that enclosing instance, and you’re right about that.

But the worst thing that we’ve seen in the above code is the fact that even if you know what you are doing with your map that you might be creating in a static context, the next developer might not notice that and refactor / remove static again. They might store the Map in some other singleton instance and there is literally no way to tell from the code itself that there might just be a dangling, useless reference to ReallyHeavyObject.

Inner classes are a beast. They have caused a lot of trouble and cognitive dissonance in the past. Anonymous inner classes can be even worse, because readers of such code might really be completely oblivious of the fact that they’re enclosing an outer instance and that they’re passing around this enclosed outer instance.

The conclusion is:

Don’t be clever, don’t ever use double brace initialisation

Did you like this article?

We have more articles about best practices in Java:

28 thoughts on “Don’t be “Clever”: The Double Curly Braces Anti Pattern

  1. Lukas — nice article. But it leaves open the obvious question: what should people do instead? (Many answers are also obvious, I’m just curious what you would say. And besides, you’ve left us with nothing to argue about.🙂 )

    • Many answers are also obvious, I’m just curious what you would say

      If you don’t want any dependencies, then just accept your fate. It’s Java. We have void methods. We can write several statements, each on a new line😉

      Map map = new HashMap();
      map.put(a, b);
      map.put(c, d);

      See? If dependencies are OK and you really don’t want to do it with more than 1 statement, use Guava (as suggested by Mikael)

      you’ve left us with nothing to argue about.

      From time to time, our articles are less opinionated and more objective🙂

      • I really wish there was a simple way to repackage some of the Guava collection classes for libraries, as I couldn’t get their ProGuard solution to work reliably. That meant that I had to reimplement MultiMap for a library specced to require only the JDK, and disliked every minute of it.

  2. And if you want a good solution to the problem, look at Guava’s ImmutableMap.of(…), ImmutableMap.Builder and ImmutableMap.builder().put(…).put(…).build()

  3. It seems my comment got swallowed by the ether, so here it goes again.

    It is best to define another method that takes a list of key-value tuples to create the map. In FunctionalJava this is:

    public static TreeMap treeMap(final Ord keyOrd, final List<P2> list)

    https://github.com/functionaljava/functionaljava/blob/master/core/src/main/java/fj/data/TreeMap.java

    Thanks for the info on class loading. This has important implications for backporting lambdas that FunctionalJava uses. I will need to think about this…

  4. Your statement that readability is the least important reason is a matter of perspective. In professional development, readability is the single most important reason. It’s also the second most important, and third, and the fourth.

    We commercial developers spend 90% of their time maintaining existing code. Readable code saves vast amounts of time during maintenance, and therefore money. Code “tricks” are a good way to turn a 30-minute maintenance task into one that takes hours or days.

    Java’s deliberate lack of language features is in fact based on that idea. As a Sun VP put it: “It is more important that Java programs be easy to read than to write.”

    Your other reasons are entirely valid and important, but I would still put readability before any of them.

    • Thanks for the bytecode contributions. Well, splitting the braces is probably not an option to those people who like the cleverness of this pseudo-DSL. It would defeat the single purpose of this practice…

  5. Been writing Java commercially for 15 years now, and sorry but I mostly disagree. A good way to waste a lot of time in development is to try to anticipate everything someone else may come along and mess up in your beautiful class. You can’t, so use good engineering to design your system to be a bit more flexible than it’s initial requirements, and hope the maintenance guys don’t suck.

    1) readability: meh, I find the pattern very readable but everybody’s got different opinions about what’s readable (see e.g. @VGR above who obviously feels a lot more strongly than most about this than most — to me 90% of code is perfectly readable after a ctl-shift-F)

    2) unless you make thousands of these, the ClassLoader can handle it, not a concern

    3) see above — you can’t fix what some future programmer may do to your code, don’t even try.

    • Thanks for your comment. I don’t think you really disagree – perhaps to tone but not to intent. Every “anti-pattern” is only a “pattern” when applied thousands of times. If you write this code only once or twice, that may be perfectly fine, just like break and continue, for instance. But some people quickly jump to the conclusion that this is an awesome way to write DSLs in Java, which will create “thousands of these” as you put it.

    • I don’t agree:

      1. There’s still some GC pressure on class loader objects. Class references are still rather heavy, regardless if they’re permanent or volatile.
      2. You can still leak internal resources to the “outside” because of the hidden enclosing instance

      It’s just bad practice to use this “pattern”.

    • I don’t see any mentioning of “PermGen” in this article. The fact that all these classes have to be loaded and will remain in memory as long as their ClassLoader is reachable didn’t change. And the class loader is the same as for the outer class. So still, each double-curly-braced initialized map will cause an overhead higher than its actual payload.

      • You’re right. There’s no explicit “PermGen” reference, but there’s this:

        you will put some unnecessary burden on your ClassLoader, which keeps references to all these class objects on your heap

        I didn’t call it “PermGen”, because that memory area was effectively removed in Java 8. Class are now placed on the “regular heap”

  6. It should be mentioned that even if we turn the method to static, the maps won’t have a reference to an outer class instance anymore, but the inner maps still have unexpected references to their outer maps, which may cause resource leaks. And this is not solvable as the whole point of the anti-pattern is to put the code into an /instance/ initializer.

    And of course, there’s an impact on Serialization. Storing these maps creates persistent references to classes which are an implementation detail. Using another map with curly brace initialization within the class can cause renumbering of the generated names of the inner classes, breaking the entire persistent form…

  7. Readability – this reminded me of a Linus quote: “Subversion used to say CVS done right: with that slogan there is nowhere you can go.” If you’re looking for readability in Java, there is nowhere you can go.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s