What's the harm in using Anonymous class?

Question

The question arose while reading a answer to this question - How do I join two lists in java. This answer gave the solution

List<String> newList = new ArrayList<String>() { { addAll(listOne); addAll(listTwo); } }; 

Reading the comments, users said it was evil and ugly and should not be used in production.

I would like to know what's the harm in using this? Why is it ugly, evil or bad to use in production?

Answer 1

Except for the already mentioned issues regarding good programming style and inheritance misuse, there is one more subtle problem - inner classes and (non-static) anonymous class instances act as closures. This means that they keep an implicit reference to the enclosing class instance. This can result in preventing of garbage collection and in the end, a memory leak.

Given an example piece of source code:

public interface Inner {     void innerAction(); }  public class Outer {      public void methodInOuter() {}      private Inner inner = new Inner() {         public void innerAction() {             // calling a method outside of scope of this anonymous class             methodInOuter();           }     } } 

What happens at compilation time, is that the compiler creates a class file for the new anonymous subclass of Inner which gets a so-called synthetic field with the reference to the instance of the Outer class. The generated bytecode will be roughly equivalent to something like this:

public class Outer$1 implements Inner {      private final Outer outer; // synthetic reference to enclosing instance      public Outer$1(Outer outer) {         this.outer = outer;     }      public void innerAction() {         // the method outside of scope is called through the reference to Outer         outer.methodInOuter();     } } 

Such capture of reference to the enclosing instance happens even for anonymous classes that never actually access any of methods or fields of the enclosing class, such as the double-brace initialized (DBI) list in your question.

This results in the fact that the DBI list keeps a reference to the enclosing instance as long as it exists, preventing the enclosing instance from being garbage collected. Suppose the DBI list happens to live for a long time in the application, for example as a part of the model in MVC pattern, and the captured enclosing class is for example a JFrame, which is quite a large class with lots of fields. If you created a couple of DBI lists, you would get a memory leak very quickly.

One possible solution would be using DBI only in static methods, because there is no such enclosing instance available in their scope.

On the other hand, I would still argue that using DBI is still not necessary in most cases. As for the list joining, I would create a simple reusable method, which is not only safer, but also more concise and clear.

public static <T> List<T> join(List<? extends T> first, List<? extends T> second) {     List<T> joined = new ArrayList<>();     joined.addAll(first);     joined.addAll(second);     return joined; } 

And then the client code becomes simply:

List<String> newList = join(listOne, listTwo); 

---

Further reading: https://stackoverflow.com/a/924536/1064809