So Clever It’s Stupid!

Sometimes you come up with a solution that seems like genius and then, on closer inspection, you realise that all you have done is impress yourself, rather than create something that has the finesse of a true coding craftsman. I recently performed such an act of ego-stroking and would like to come clean. I think it’s worth explaining why what I did was not as clever as I thought and how I fooled myself into thinking it was the neatest solution. Hopefully there’s a lesson here for others.

The Problem

Firstly, let me explain the problem I was trying to solve. I had a class which needed to be populated with some data on construction. I was going to use it as a Spring bean and wanted the data to be populated at run time from whichever configuration Spring happened to have. So far so good.

The class had a constructor thus:

public MyClass(Set<String> theData) { ... }

This was easy to use from unit tests as I could instantiate objects of the class with any data I liked and check its behaviour. So far so good.

When it came to populating the data for the runtime version, which I was going to test via a test spring configuration, I needed to somehow get Spring to produce a set of Strings, but I wanted the set to be read out of a plain old Java properties file, or even an environment variable.

I wanted to know how to populate a Set in the constructor of a Spring bean by Spring reading from a properties file. I’ll say it another way for anyone Googling this. How do you get a Set to be instantiated by Spring via Autowiring and can Spring execute any helper functions to convert the data from the properties file into the right format.

One extra nuance. The internal representation of the class would prefer that all Strings were in lower case and I didn’t want the constructor to have to replace the set it was given with a sanitised one. Why I didn’t want this isn’t clear to me.

There is some magic you can use.

@Autowired
public void MyClass(
    @Value("#(T)(com.company.MyUtils).splitString('${my.property}'.toLowerCase())")
    Set<String> theData
) { ... }

Let me break this down. The @Value is used in conjunction with @Autowired on this constructor to tell Spring to automatically instantiate the class by inserting some value from the rest of the environment into the parameter of the constructor.

I had already written a helper function in com.company.MyUtils which could split the string, and String has the toLowerCase function for dealing with my case issue.

${my.property} would refer to a property or environment variable my.property that I would have injected into the runtime via a properties file etc.

On the face of it, I’ve nailed this. I have my constructor getting the perfect input – a set of lowercase strings read from a single string in the properties (assume my split function broke up a comma delimited string, because that’s what it did), automagically created by this nifty Expression Language syntax. Result!

What did I do WRONG?

  • The constructor wasn’t protecting the internals of the class from incorrect data being passed in – it should be dealing with the case of items in the set
  • The logic for converting from a single string to a set is not unit tested – it is integration tested, but that puts more work into the integration tests than they ought to have
  • The logic for the conversion is written in a kind of meta-language – it’ll all be running Java, but this string saying what java to call at runtime is not really refactorable
  • It’s a surprise when you read it

What would have been simpler?

@Autowired
public MyClass(@Value("${my.property}") String byProperty) {
    this(MyUtil.split(byProperty.toLowerCase()));
}

In this constructor we’re using the autowiring and injection of properties technique, but we’re expecting the constructor to convert from the mixed case delimited string into the set format. Not only that, but it’s now unit testable and you can read better what it’s doing.

Why did I shy away from this?

This is partly down to tunnel vision – I had decided what the constructor was and was trying to use the one I already had. In addition, I think sometimes you feel like you don’t want to pollute a class with knowledge of the outside world. I felt my class was unaware of the way properties might be represented, so didn’t have to have the algorithm in it. Except it did. The algorithm ended up in an annotation in the form of a meta-Java, unreadable and unrefactorable.

Lessons learned

  • Keep it simple.
  • Don’t be clever.
  • Admit when your class needs to do more.
  • Admit when your class is already doing more.
  • Put code in Java, not in annotations.
  • The goal should be refactorability and testability not self-congratulation
Advertisements

Software developer, stand-up comedian, musician, writer, jolly big cheer-monkey, skeptical thinker, Doctor Who fan, lover of fine sounds.

Tagged with: ,
Posted in Uncategorized

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: