Getting rid of those damn globals

 I've just (well, yesterday) finished a nice little piece of refactoring which I felt was an interesting problem. I enjoyed it anyway, so I'll share it.

We have a number of home grown library packages that we use fairly extensively around our applications. One of the more useful ones is a configuration library. It has a handy little abstract class, AbstractConfiguration, that's designed to read and validate a config file based on the various configuration items you need.

That isn't terribly clear, but our tests should document it.

	public class AbstractConfigurationTest {
	private PropertiesConfiguration propertiesConfig;
	@Rule
	public ExpectedException expectedException = ExpectedException.none();
	@Before
	public void setup() throws Exception {
	propertiesConfig = new PropertiesConfiguration();
	}
	@After
	public void after() {
	AbstractConfiguration.clearEntries();
	}
	@Test
	public void shouldSetEntryValue() throws Exception {
	StringConfigurationEntry entry = AbstractConfiguration.define(new StringConfigurationEntry("entry"));
	propertiesConfig.setProperty("entry", "value");
	new AbstractConfiguration(propertiesConfig){};
	assertEquals("value", entry.getValue());
	}
	@Test
	public void shouldErrorIfExtraKeysInConfigFile() throws Exception {
	expectedException.expect(ConfigurationException.class);
	expectedException.expectMessage("Key [shouldnt] with value [behere] is not defined");
	AbstractConfiguration.define(new StringConfigurationEntry("entry"));
	propertiesConfig.setProperty("entry", "value");
	propertiesConfig.setProperty("shouldnt", "behere");
	new AbstractConfiguration(propertiesConfig){};
	}
	@Test
	public void shouldLeaveDefaultValueInPlaceIfPropertyIsMissing() throws Exception {
	StringConfigurationEntry entry = AbstractConfiguration.define(new StringConfigurationEntry("entry", "default"));
	new AbstractConfiguration(propertiesConfig){};
	assertEquals("default", entry.getValue());
	}
	@Test
	public void shouldThrowConfigurationExceptionAnyIfPropertyIsInvalid() throws Exception {
	expectedException.expect(ConfigurationException.class);
	expectedException.expectMessage("Configuration is invalid, the following problems were detected:\n\tmy.entry value [] is not valid");
	AbstractConfiguration.define(new StringConfigurationEntry("my.entry", "default"));
	propertiesConfig.setProperty("my.entry", "");
	new AbstractConfiguration(propertiesConfig){};
	}
	@Test
	public void shouldThrowConfigurationExceptionIfAnyMandatoryEntryIsMissing() throws Exception {
	expectedException.expect(ConfigurationException.class);
	expectedException.expectMessage("Configuration is invalid, the following problems were detected:\n\trequired key [my.entry] not found");
	AbstractConfiguration.define(new StringConfigurationEntry("my.entry"));
	new AbstractConfiguration(propertiesConfig){};
	}
	}

view raw InitialAbstractConfigurationTest.java hosted with ❤ by GitHub

Oh dear, that's hardly documenting it. In fact, I'd go so far as to say that's making it less clear. Oh well, let's start out by making our test document how you'd be expected to implement an AbstractConfiguration by including one in the test class:

	private static class ConfigurationExample extends AbstractConfiguration {
	private static final StringConfigurationEntry ENTRY_WITH_DEFAULT = define(new StringConfigurationEntry("entry.that.defaults", "default"));
	private static final StringConfigurationEntry ENTRY_WITHOUT_DEFAULT = define(new StringConfigurationEntry("entry"));
	private static final IntegerConfigurationEntry INTEGER_ENTRY = define(new IntegerConfigurationEntry("integer"));
	public ConfigurationExample(Configuration configuration) throws ConfigurationException {
	super(configuration);
	}
	public ConfigurationExample(Properties properties) throws ConfigurationException {
	super(properties);
	}
	public String getEntryWithoutDefault() {
	return ENTRY_WITHOUT_DEFAULT.getValue();
	}
	public String getEntryWithDefault() {
	return ENTRY_WITH_DEFAULT.getValue();
	}
	public Integer getIntegerEntry() {
	return INTEGER_ENTRY.getValue();
	}
	}

view raw InitialConfigurationExample.java hosted with ❤ by GitHub

And then change the tests so that they test the implementation. Here's how the change affects the first two tests:

	private ConfigurationExample configurationExample;
	@Before
	public void setup() throws Exception {
	propertiesConfig = new PropertiesConfiguration();
	propertiesConfig.setProperty("entry", "value");
	}
	@Test
	public void shouldSetEntryValue() throws Exception {
	propertiesConfig.setProperty("entry", "not the value in setup");
	configurationExample = new ConfigurationExample(propertiesConfig);
	assertEquals("not the value in setup", configurationExample.getEntryWithoutDefault());
	}
	@Test
	public void shouldErrorIfExtraKeysInConfigFile() throws Exception {
	expectedException.expect(ConfigurationException.class);
	expectedException.expectMessage("Key [shouldnt] with value [behere] is not defined");
	propertiesConfig.setProperty("entry", "value");
	propertiesConfig.setProperty("shouldnt", "behere");
	new ConfigurationExample(propertiesConfig);
	}

view raw TestUsingConfigurationExample.java hosted with ❤ by GitHub

Anyway, as you can now see, we implement an AbstractConfiguration by extending it, and defining a number of AbstractConfigurationEntries, which we create getters for. The entries themselves contain validation rules (I could, for example, have shown you an IntegerRangeConfigurationEntry or a PositiveDoubleArrayConfigurationEntry - you get the picture ...) and those entries are validated on construction. If any validations fail, an exception is thrown with a meaningful message describing all the problems found. We find it handy.

The problem I ran into was that, I haven't shown you this yet, but the list of entries is a static variable on the abstract class. Since any mandatory fields that are missing will cause validation to fail, you can't have two different implementations in the same JVM. This hasn't proved a problem before - how many configurations does one app need? - but I now needed two. I could, I suspect, have worked around the issue, but that didn't appeal. There was a nasty smell hanging over the library.

First step, writing a test. This was hardly taxing, first I need another implementation in my test:

	private static class SecondConfiguration extends AbstractConfiguration {
	private static final StringConfigurationEntry DIFFERENT_ENTRY = define(new StringConfigurationEntry("different.key"));
	public SecondConfiguration(Configuration configuration) throws ConfigurationException {
	super(configuration);
	}
	public String getDifferentEntry(){
	return DIFFERENT_ENTRY.getValue();
	}
	}

view raw SecondConfiguration.java hosted with ❤ by GitHub

Followed by a test case:

	@Test
	public void shouldBeAbleToCreateTwoDifferentConfigurationsInOneJVM() throws Exception {
	new ConfigurationExample(propertiesConfig);
	PropertiesConfiguration secondPropertiesConfig = new PropertiesConfiguration();
	secondPropertiesConfig.addProperty("different.key", "value");
	SecondConfiguration secondConfiguration = new SecondConfiguration(secondPropertiesConfig);
	assertEquals("value", secondConfiguration.getDifferentEntry());
	}

view raw FailingTest.java hosted with ❤ by GitHub

Simple enough, and the test fails nicely. Ok, so what does AbstractConfiguration actually look like?

	public abstract class AbstractConfiguration {
	private static List<AbstractConfigurationEntry<?>> entries = new ArrayList<AbstractConfigurationEntry<?>>();
	protected static <T extends AbstractConfigurationEntry<?>> T define(T entry) {
	entries.add(entry);
	return entry;
	}
	public AbstractConfiguration(Configuration configuration) throws ConfigurationException {
	populateEntries(configuration);
	}
	private void populateEntries(Configuration configuration) throws ConfigurationException {
	List<String> problemMessages = initialiseEntries(configuration);
	if (! problemMessages.isEmpty()) {
	String errorMessage = buildExceptionMessage(problemMessages);
	throw new ConfigurationException(errorMessage);
	}
	}
	}

view raw InitialAbstractConfiguration.java hosted with ❤ by GitHub

I've not shown you the detail of how it initialises and validates the entries, as there's already too much code in this post, but it ought to be fairly obvious.

Since the problem is obviously that entries is static, we can fix it by removing it's static nature and finding some other way to populate it. This obviously means that define can't add the entry being defined to the list, making it a pointless method. We'll deprecate it so we don't piss the rest of the teams using this off too much, and remove it in version 1.2

	private List<AbstractConfigurationEntry<?>> entries = new ArrayList<AbstractConfigurationEntry<?>>();
	/**
	*
	* {@link AbstractConfiguration} now uses reflection to build its list of entries<p>
	* You no longer need to call define to add your {@link AbstractConfigurationEntry}s to the list to be validated<p>
	*
	* This method will be removed in the next version
	*
	*/
	@Deprecated
	protected static <T extends AbstractConfigurationEntry<?>> T define(T entry) {
	return entry;
	}

view raw ChangeEntriesAndDeprecateDefine.java hosted with ❤ by GitHub

So, how to build entries dynamically? Well, we could take a step back, towards the 0.1 version, and build a list in a static block and pass that in to the constructor, but we moved away from that for a reason - it was rubbish. Since this isn't a class that you're going to be creating all over the place, why don't we use reflection? All of our entries are already constants, it's not terribly hard to get at them via reflection, we can just call this.getClass().getDeclaredFields() to get only the fields declared by our implementation, check to see if they're an AbstractConfigurationEntry, and if so, add them into our fields member. All of which can happily be done on construction. Giving us a new populateEntries method:

	private void populateEntries(Configuration configuration) throws ConfigurationException {
	buildEntriesList();
	List<String> problemMessages = initialiseEntries(configuration);
	if (! problemMessages.isEmpty()) {
	String errorMessage = buildExceptionMessage(problemMessages);
	throw new ConfigurationException(errorMessage);
	}
	}
	private void buildEntriesList() {
	for (Field field : this.getClass().getDeclaredFields()) {
	if(isAConfigurationEntry(field) ) {
	addFieldToEntries(field);
	}
	}
	}
	@SuppressWarnings("unchecked")
	private boolean isAConfigurationEntry(Field field) {
	Type type = field.getGenericType();
	if (type instanceof Class) {
	return AbstractConfigurationEntry.class.isAssignableFrom((Class) type);
	}
	return false;
	}
	private void addFieldToEntries(Field field) {
	field.setAccessible(true);
	try {
	AbstractConfigurationEntry<?> fieldObject = (AbstractConfigurationEntry<?>)field.get(null);
	entries.add(fieldObject );
	} catch (IllegalAccessException e) {
	throw new RuntimeException(e);
	}
	}

view raw NewPopulateEntries.java hosted with ❤ by GitHub

And there you have it. One less smell in our code base, and one more developer feeling slightly smug.