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.

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:

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

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:

Followed by a test case:

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

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

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:

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