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

Posted on by lukaseder

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() {
}
  • 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 usage of this syntax

    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:

    Published by lukaseder

    I made jOOQ

    37 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. :-) )

      Reply

      1. 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 :-)

        Reply

        1. 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.

          Reply

    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()

      Reply

      1. That’s clever. Nice usage of type inference as well! Perhaps you should suggest that to the Guava teams (I haven’t checked whether they already included such API)

        Reply

    4. 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…

      Reply

    5. 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.

      Reply

      2. I agree mostly. Readability is important and java has precious few ways to initialize collections. In a perfect world it would all be external data read by an engine of some sort but apparently I don’t live in a perfect world.

        So wouldn’t executing the initialization in a static method fix the problem with the memory allocation? That could easily be part of the pattern. Also the extra classes are rarely an issue–I’ve had class count issues on some platforms but it’s usually rare–and as VGR mentions, in our realm readability is ALWAYS first, all optimization second and only if there is a proven need.

        So if these are the only two problems with the pattern, is there any reason to call it an anti-pattern… at least for enterprise development? (Personally I think there is a surprise factor too–you have to be sure your team isn’t stuck trying to figure out what you did–but that’s true of a lot of stuff, I’ve worked with teams who thought that ? : was too “Surprising” to use.)

        Reply

        1. You say “enterprise development” as though that was something inferior ;-)

          Yes, I rest my case. It’s an anti pattern. especially in an enterprise environment, where one clever developer who does this once in a static initialiser will trigger all juniors to copy this pattern everywhere.

          Reply

    6. Because of this, I decided to just check the bytecode and realise this should really just be done by either splitting the braces, since double braces don’t really show what actually happens, or by just not being lazy and typing the field name with a dot after it.

      BTW, here’s the bytecode I used:

      Normal class: http://pastebin.com/pCgNWt36
      Anonymous inner class: http://pastebin.com/2R0bHund

      Reply

      1. 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…

        Reply

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

      Reply

      1. 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.

        Reply

      1. 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”.

        Reply

      2. 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.

        Reply

        1. 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”

          Reply

    10. 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…

      Reply

    11. 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.

      Reply

    12. Nice article. Good reasons for not to use double braces.
      Cannot find a nice way to declare multiple parameters for enum. Constructor with more than 3 parameters doesn’t look nice and double braces looks like a solution.
      What do you think about it?

      public enum Animal {
  Dog {{
    canFly = false;
    canRun = true;
    canBark = true;
  }},
  
  Eagle {{
    canFly = true;
    canRun = false;
    canBark  = false;
  }}

  protected boolean canFly;
  protected boolean canRun;
  protected boolean canBark;

 public boolean canFly()....
}
      Reply

      1. Interesting approach. This works nicely, because enum literals are really static final members, and as such cannot capture any outer scope. So the article doesn’t really apply there. Really funky syntax! :)

        Reply

    13. I really hate kittens. So if double brace initialisation helps against them I will have to use it – even if I never was going to before… :)

      Reply

    15. I’d say it’s slightly misleading to say “One type per instance”. An instance usually refers to an object. It sounds like a new type is created each time `new` is executed, which is obviously not the case.

      Perhaps “one new type per double brace occurrence” or something like that would be better.

      Also, is an extra class considered bad? I’ve never seen anyone argue that “your program have too many classes”. Sure, these types are anonymous, but that seems like a common pattern anyway? I’ve never seen anyone complain about things like `new Runnable() { System.out.println(“Hello”); }`

      Reply

      1. Yeah, fair enough. The intent here was to say “one type per usage of this syntax”, i.e. per “syntax instance”. But you’re right, better be clear.

        It’s not the end of the world to add one more class, but if you have a big application, is it really worth loading, say, 2000 extra classes just to have some convenience over map creation all over the place? It might make a difference, it might not.

        Reply

    Leave a Reply