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?

---

Note: Asked it as a question because, the post referenced is too old (2008) and the answerer is away since several months.

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

Answer 2

The "ugly" and "do not use in production" comments refer to this specific use of anonymous classes, not to anonymous classes in general.

This specific use assigns newList an anonymous subclass of ArrayList<String>, an entirely new class created with a single purpose in mind - namely, initializing a list with the content of two specific lists. This is not very readable (even an experienced reader would spend a few seconds figuring it out), but more importantly, it can be achieved without subclassing in the same number of operations.

Essentially, the solution pays for a small convenience with creating a new subclass, which may result in problems down the road, for example, in situations when you try to persist this collection using an automated framework that expects collections to have specific types.

Answer 3

This particular use of anonymous classes has several problems:

1. it's a little-known idiom. Developers that don't know it (or know it an don't use it a lot) will be slowed down when reading and/or modifying code that uses it.
2. it's actually misusing a language feature: you're not trying to define a new kind of ArrayList, you just want some array list with some existing values in it
3. it creates a new class that takes up resources: disk space to hold the class definition, time to parse/verify/... it, permgen to hold the class definition, ...
4. even if the "real code" is slightly longer, it can easily be moved into an aptly-named utility method (joinLists(listOne, listTwo))

In my opinion #1 is the most important reason to avoid it, closely followed by #2. #3 is usually not that much of a problem, but should not be forgotten.

Answer 4

Because you don't need a separate subclass - you just need to create a new ArrayList of the normal class, and addAll() both lists into it.

Like so:

public static List<String> addLists (List<String> a, List<String> b) {
    List<String> results = new ArrayList<String>();
    results.addAll( a);
    results.addAll( b); 
    return results;
}

It's evil to create a subclass, that isn't needed. You don't need to extend or subclass behaviour - just change data values.

Answer 5

It's not a bad approach per se, say, in performance or anything like that, but the approach is a bit obscure and when using something like this, you always (say, 99%) have to explain this approach. I think that's one of the biggest reasons not to use this approach, and while typing:

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

is a little more typing, it's a bit easier to read, which helps a lot in understanding or debugging code.

Answer 6

In your example it really looks evil and ugly, at least to me - it's difficult to understand what's going on in the code. But there are some patterns of using anonymous classes that people are used to because they see them very often, eg

    Arrays.sort(args, new Comparator<String>() {
        public int compare(String o1, String o2) {
            return  ... 
        }});

I would call the above a best practice case.