JAW Speak

Jonathan Andrew Wolter

Using (These) Anonymous Inner Classes is Probably Too Clever for Your Own Good

with 5 comments

Reading time: 3 – 5 minutes

Sometimes it is tempting to have less verbose Java code and be clever. Say you have an object you need to build and set several properties on it, then use it. This way looks clever, but is a bad idea – especially in production code. Read on for why.

  	TripChoice myTrip = new TripChoice() {{
                        // look at these clever initialization blocks!
			setDestination("San Francisco");
			setAvailableRoutes(new HashSet() {{
				add("LAX-SFO");
				add("BUR-SFO");
				add("ONT-SFO");
			});
	   }};

This is a huge problem because it can cause a NotSerializableException or create a memory leak. How? Because with that clever anonymous class and initialization block, there is an implicit reference to the outer class’ instance. That outer class will not be garbage collected so long as there is one reference to the anonymous class. Worse, if we ever

Here’s the typical way to do it.

                // blah blah, this is typical, and a rather boring looking great big block of text.
		// (That's also probably scattered over several places in your code when you build these objects).
		TripChoice myTrip = new TripChoice();
		myTrip.setDestination("San Francisco");
		Set availableRoutes = new HashSet();
		availableRoutes.add("LAX-SFO");
		availableRoutes.add("BUR-SFO");
		availableRoutes.add("ONT-SFO");
		myTrip.setAvailableRoutes(availabileRoutes);

Here’s why this improved version is even not the best:

  • It uses setters. Setters allow your object to be constructed in an inconsistent state. There are some situations setters are fine (i.e. form backing objects) but often your code is more clear without setters. In this particular case the setters aren’t egregious, but I still consider them a smell and much more verbose way that constructor injection. Read more about the problem with setter injection here
  • It is verbose.
  • It is also mutable (often a negative), again a problem due to the use of setters.

So what’s the best way to do it? I have found constructor injection and in our particular example use of Google Collections. As for seeing an implementation of the best solution – just leave a comment.

Below is a full example you can run that illustrates the hard to catch serialization bug.

package com.jawspeak;
 
import com.google.common.collect.Lists;
import static org.junit.Assert.*;
import org.junit.Test;
 
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.NotSerializableException;
import java.io.ObjectOutputStream;
import java.util.ArrayList;
import java.util.List;
 
/**
 * Understands the danger of anonymous subclasses for lists.
 */
public class TooCleverForOurOwnGoodTest {
 
        @Test(expected = NotSerializableException.class)  // UPDATE: Thanks for the reminder from Dennis, below
	public void serializableDangerBugFromPatricksCleverness() throws IOException {
		// you think a List would be serializable, which it is. And so are Strings.
		final String myString = "3";
		List strings = new ArrayList() {
			add("one");
			add("two");
			// However, because there is an anonymous subclass of ArrayList created,
			// there is an implicit reference to the outer class, which is **not serializable**.
			// This is a nice feature, which lets us have closure-ish syntax in java, such as
			// the following reference to the outer myString.
			add(myString); // It will throw the exception even without this reference.
		}};
 
		// call your method that requires args to be serializable
		ByteArrayOutputStream baos = new ByteArrayOutputStream();
		ObjectOutputStream oos = new ObjectOutputStream(baos);
		oos.writeObject(strings);
		// The particular bug I had was even worse, because we were creating leaking a reference
		// through that List's implicit reference to the outer object.
	}
 
	@Test
	public void serializableDangerBugSafeButNotClever() throws IOException {
		// A List is serialiazable. And so are Strings.
		List strings = new ArrayList();
		strings.add("one");
		strings.add("two");
		// No longer is there a reference to the outer class.
 
		// call your method that requires args to be serializable
		ByteArrayOutputStream baos = new ByteArrayOutputStream();
		ObjectOutputStream oos = new ObjectOutputStream(baos);
		oos.writeObject(strings);
	}
 
	@Test
	public void serializableDangerBugSafeAndNiceLooking() throws IOException {
		// A List is serialiazable. And so are Strings.
		List strings = Lists.newArrayList("one", "two");
 
		// call your method that requires args to be serializable
		ByteArrayOutputStream baos = new ByteArrayOutputStream();
		ObjectOutputStream oos = new ObjectOutputStream(baos);
		oos.writeObject(strings);
	}
}
Bookmark and Share

Written by Jonathan

March 10th, 2009 at 11:33 pm

Posted in Uncategorized, code, java, testability

Tagged with , ,

5 Responses to 'Using (These) Anonymous Inner Classes is Probably Too Clever for Your Own Good'

Subscribe to comments with RSS or TrackBack to 'Using (These) Anonymous Inner Classes is Probably Too Clever for Your Own Good'.

  1. This effect has nothing to do with the *clever* initializer braces (which cause the code inside to be inserted immediately after the super() call in the constructor)… you get the same effect by simply anonymously extending ArrayList.

    dirty

    11 Mar 09 at 3:14 am

  2. I use java.util.Arrays.asList(T…) for fixed-size lists. Third variant is good enough too.

    Oleg

    11 Mar 09 at 6:17 am

  3. How do you get a memory leak?

    Henry

    12 Mar 09 at 11:42 am

  4. optimization Tips…

    Using (These) Anonymous Inner Classes is Probably Too Clever for Your Own Good at JAW Speak…

    optimization Tips

    29 Aug 17 at 9:16 am

  5. bicycles frequently…

    Using (These) Anonymous Inner Classes is Probably Too Clever for Your Own Good at JAW Speak…

Leave a Reply