You can lead a horse to water...

Although the other developers on the refactoring project agree that the code needs to be made better sometimes they don’t seem to bother to make changes in a way that improves things…

This week a major new piece of functionality was completed. I helped with merging the branch back onto main and started to sort out the release; then I realised none of the test harnesses compiled anymore…

The developer responsible for the new functionality had changed an interface and then failed to update the tests to test the change. I started to investigate the change to find out why it was required.

The trade entry screens have validation for the user entered values. The validation data comes from a variety of sources but the validation interface is consistent. You call a method on a validator and it optionally pops up a gui with the nearest misses of the valid values if the value supplied is invalid. The call returns the key to the new value selected. Except now it does more…

Some validation data is multiple column. The first column is the ‘key’ and is the actual value that’s stored, the other columns are just convenient mappings to the key. For example, you might have customer codes and allow the user to search the code itself, a long name, a short name, a code from another system, etc. The user can customise each validation search to allow them to just search a particular type of value; for the customer example a user who always uses reuters codes would check a box to have his selections validated against only the reuters codes.

What the developer had done is use the validation data store as a kind of static data lookup store. He added some fields related to the data he was validating to the validator as additional columns and then hacked a change into the validation interface so that it returned him all the columns rather than just the key. He then propogated this interface change through all the code that used the interface and simply ignored the extra results of the call in 99% of the call sites. In the place where he needed the data he used it…

Leaving aside the confusion that the users might feel on seeing the selection box this extra ‘validation data’, the change was ugly. It was a hack and, what’s more, it broke the tests. What was annoying was that it was obviously just a hack that had been thrown in because it looked like the easiest way to deal with this new issue. What’s more, the validator itself was accessible from the location that he needed to do his lookup from - it shouldn’t be but we have this horrible singleton application object that too many pieces of code are coupled to. The validator has a pretty wide public interface but also implements a narrower ‘user validation’ interface. He’d extended the user validation interface to provide a side effect for a spectial case scenario and then ignored the results everywhere else… He could have used the public interface of the validator to do the work.

I looked at what he wanted to do, reworked the code so that he did it at the point he required it and using the public interface of the validator rather than extending the user validation interface and removed all the hacks. We went from >20 files changed to 3 and the tests still ran.

I gave him and our boss the option of fixing the tests to work with his proposed changes or using my alternative solution. We went with the alternative.

Now, what will the users say about all that extra rubbish in their validation dialog…