JAW Speak

Jonathan Andrew Wolter

Archive for the ‘testability’ tag

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);
	}
}

Written by Jonathan

March 10th, 2009 at 11:33 pm

Posted in Uncategorized, code, java, testability

Tagged with , ,

Arguing for software testing in difficult environments

with 3 comments

Reading time: 4 – 6 minutes

I’m a ThoughtWorker. ThoughtWorks is changing the way that enterprise software is delivered. And with that we take firm stances on heavily debated topics. In previous jobs I’ve tried to push test driven development, unit testing, code coverage metrics, continuous integration… all controversial ‘best practices’. Results were mixed.

A few weeks ago I was at a large web 2.0 social networking site working with selenium grid automation. They were great clients, fully receptive to automated testing. Next week I’ll be heading to another leading internet company to work triage:

  1. Work on troubled teams whose code is poorly tested
  2. Enable groups to test legacy code
  3. Attempt to spread a pervasive test driven mindset.

I’m joining a senior team of ThoughtWorkers and in preparation I’ve thought of various arguments I’ve heard (or used myself) against testing code. I’ll be challenged working with these very experienced people, but I am eagerly looking forward to the experience.

Argument #1

Adding all these tests only makes more code to maintain, debug, and write. This can’t be good – I want less work, not more!

Rebuttal: Would you agree code and requirements often change? Would it be valuable if something could automatically and accurately catch bugs introduced with changes? How about if the original developers are no longer on the project? Testing can enable less work — in a dynamically changing environment. Immediately the work is greater, but over time it is less.

Argument #2

Ok, fair enough, there are some good reasons to do this. BUT, when I want to change something later, I now have two points of failure – the code I’m changing, and all the tests that depend on that code. I haven’t really bought myself all that much security, because if my tests don’t catch the problem well, I’m just as hosed as if I had no tests. Source: first comment from here
Rebuttal:

  1. You get better and faster with tests the more you write them.
  2. By writing tests you further understand the business domain and craft a better thought out solution.
  3. Whenever making changes in the future you actually have 1 + n points of failure. That which you are changing plus the other interacting systems within the code. By writing tests you will automatically catch the interactions, as well as the initial point of failure. Sure the tests need maintaining, but now with two things to maintain, you catch (almost all) these failure points.

Argument #3

I generally think testing is a good idea. But I’m stressed out, I’ll get to it later… tomorrow I’ll add tests… as soon as I get this working
Rebuttal: Take a page from Agile Software Development: Principles, Patterns, and Practices by Robert C. Martin.. He argues for refactoring but since the two concepts are so tightly entwined, I think the argument applies here:

“Refactoring is like cleaning up the kitchen after dinner. The first time you skip it, you are done with dinner more quickly. But that lack of clean dishes and clear working space makes dinner take longer to prepare the next day. This makes you want to skip cleaning again. Indeed, you can always finish dinner faster today if you skip cleaning, but the mess builds and builds. Eventually you are spending an inordinate amount of time hunting for the right cooking utensils, chiseling the encrusted dried food off of the dishes, and scrubbing them down so that they are suitable to cook with. Dinner takes forever. Skipping the cleanup does not really make dinner go faster.”

Skipping testing does not really make software development faster, because changes are guaranteed. (You’ll have to cook another dinner eventually). Without an easy way to baseline and build upon existing code, time is spent bugfixing that could instead be adding features or writing new tests.

Argument #4

I’m awesome. I don’t write bugs. So I don’t need tests.
Rebuttal: Great. I’m excited to be working with you. I’m sure I’ll be able to learn a great deal. I imagine you like fresh challenges. And in six months or a year this current project won’t be as interesting to you as it is today. In fact, you’ll be on to something more challenging and worthy of your awesomeness.

So that means someone else — possibly a junior developer — will be maintaining and working on this code. Without tests, they will be frequently seeking your assistance and guidance to prevent bugs. This is not what you want, is it? You want new challenges, not constantly being hassled by old code. So write the tests now to ensure your ego and intellect can move forward to bigger and better things.

Elaborating and paraphrasing, Neal Ford argued at eRubycon 2007:

Now I look at not testing as professionally irresponsible. I’m paid to create software, to deliver on a client’s business needs. If I don’t rigorously and automatically ensure I have accomplished this with the minimum amount of bugs — I am committing malpractice.

What arguments have you encountered, and how have you responded?

Written by Jonathan

October 17th, 2007 at 3:02 am

Posted in Uncategorized

Tagged with