Monday, August 31, 2009

protected variables

I don't like protected variables. I hardly ever use them, and generally don't think they are useful. Making variables protected (or default access) exposes them to many of the same problems that public variables have, yet while we all (should) shutter at the thought of public instance variables, we generally don't as much for protected.

protected variables allow for multiple entry points. take the case:

public class MyClass {
protected int myProtectedVar;
private int myPrivateVar; // setters and getters omitted, but exist
}

public class Client {
private final MyClass mc = new MyClass();

public void doSomething1() {
mc.myProtectedVar = -1;
mc.setPrivateVar(-1);
}

public void doSomething2() {
mc.myProtectedVar = -2;
mc.setPrivateVar(-2);
}
}


Imagine multiple classes accessing the variables this way. At some point in development, I realized I have a bug where these variables are being set to the incorrect values so now I want to log the sets of the variables.


// easy case for the private variable
public void setPrivateVar(int i) {
LOGGER.debug("Variable i set to " + i);
this.myPrivateVar = i;
}


It should be clear now that the protected variable has no single entry point to log. While this is a trivial example, it can be extended to other concepts, such as validating inputs. It becomes even more of a problem if the variable is say a List. Now the protected variable is exposing the actual list, as opposed to returning an unmodifiable copy of it. Tracing problems gets extremely difficult, very quickly.

Exposing protected variables also discourages the "tell, don't ask" principle. By accessing the variable, you are querying the state of the object presumably to do something based on that state. Rather, you should be telling the object what to do and it will look at its own, private state.

I often hear arguments that it's ok to use protected (or even public) variables for final fields in lightweight data containers (or data transfer objects). I still don't like this because you lose the single entry point to do any sort of action on the data (validation, logging, etc). I generally find that exposing protected variables is a smell that too much internal state is being exposed.

Sunday, August 23, 2009

Spring Managed Singletons for Testability

I was recently writing some code that launched and external application and I never wanted to have more than one instance of this application running at once, so I immediately thought to make it a singleton. So, I started out with typical singleton code:

public class MySingleton {
private static final MySingleton INSTANCE
= new MySingleton();
private MySingleton() {
// initialization code here
}
public static MySingleton getInstance() {
return INSTANCE;
}
public Object getSomethingUseful() {
return somethingUseful;
}
}

I had a couple of classes that used this singleton, typically something like this (the method below doesn't do anything practical, it is just used for illustration):

public class SingletonUser {
public void doSomething() {
MySingleton s = MySingleton.getInstance();
Object o = s.getSomethingUseful();
if ( o == null ) {
throw new NullPointException("error");
}
System.out.println("Got the object o");
}
}

The test for the class looks something like:

public class SingletonUserTest {
private SingletonUser su;
@Before
public void setUp() {
su = new SingletonUser();
}
@Test(expected=NullPointerException.class)
public void testDoSomethingWhenGetSomethingUsefulIsNull() {
// do something to make
// su.getSomethingUseful to
// return null
su.doSomething();
fail("NullPointerException expected");
}
@Test
public void testDoSomethingWhenGetSomethingUsefulIsNotNull() {
// do something to make su.getSomethingUseful
// to return something non-null
// redirect System.out to a stream so we
// can test its contents
su.doSomething();
// do some asserts to make sure the stream
// was properly printed
}
}

So what's the problem with this? The line in the testDoSomethingWhenGetSomethingUsefulIsNull that sets the singleton object make sure O returns null. First, the test doesn't ensure that the singleton is reset back to the default state. The singleton may not provide this method (nor should it need to). Second, the singleton may not have easy methods for testing all of the conditions required. A typical solution is to add methods that are used for testing only, which I don't like at all. So what should you do?

Inject the singleton like you would any other bean in Spring and let Spring manage the singleton.

public class SingletonUser {
// A reference to the singleton
private MySingleton s;
public SingletonUser(MySingleton s) {
this.s = s;
}
public void doSomething() {
Object o = s.getSomethingUseful();
if ( o == null ) {
throw new NullPointException("oops");
}
System.out.println("Got the object o");
}
}

Now this class is much easier to test. You can mock out the MySingleton like you would any other class, but in production, the spring bean file would like something like:

<bean id="MySingleton" class="my.pack.MySingleton"
scope="singleton"/>
<bean id="SingletonUser"
class="my.pack.SingletonUser">
<constructor-arg type="my.pack.MySingleton"
ref="MySingleton"/>
</bean>

Now the nastiness of the getInstance calls will be removed and the code is easily tested.

Tuesday, August 18, 2009

Collective Code Ownership

Whether you're practicing agile, XP, or even waterfall, I'm a big believer in collective code ownership. Few things bother me more than another developer asking "Hey, is it okay if I modify your code?" with the exception of other developers getting angry when I modify "their" code.
The general rationale I get from non-believers is that someone changing the code may not know what they are doing and thus the code may break. I have a couple of problems with this:
1. Unit tests should be testing the code, especially if you're worried about it breaking. When any developer makes changes, if the tests are all still passing, you should be able to safely assume the code is working properly.
2. Developers generally aren't just going around changing code without knowing something about it (or at least they probably shouldn't be). And if the developer doesn't know too much about the area, they will (or should) ask someone that does know about it. In my opinion, it's definitely acceptable to have a person or people that are the points of contact for a given area of code. If you need help with it, ask them, but points of contact and code owners are very different.

Individual code ownership promotes blaming/praising individuals. While it is certainly important to know who is contributing what, a software team succeeds or fails as a whole. Sure, different people may contribute more than others, but if the product is not successful, the whole team fails. And that's really the bottom line.

Sunday, August 16, 2009

Why You Should Test "Prototype" Code

Too often developers create "prototype" code that makes it into the codebase without being tested. How does this happen? Maybe a developer is just testing one of many possible solutions and doesn't want to test code that he might throw away (hint: this sounds like a good place for TDD). Another common place I've seen this happen when you're prototyping ideas for a end-user to try out, and you don't even know if they'll like it, so you don't bother with the tests up front. I've seen this latter case happen most frequently when requirements are not well defined and the customer isn't entirely sure what they want.
Regardless of how this code gets in there, once it's part of your production code baseline, it's not prototype code anymore, it's production code. In an ideal world, we'd write all of our tests up front and we'd never have any untested code. While this doesn't always happen, when we do put that untested code in the baseline, be sure to go back and write the tests for it before moving on to the next feature. You and future developers will be glad you did.