Unsound FX

| 0 Comments

We've been moving pretty quickly on the refactoring project. We had got to the point where we were doing at least two releases a week. Generally we would include user requested fixes in the first release and refactored code in the second. It started to become a little hectic. Last week we decided to slow things down a little so that we could regroup...

One aspect of the project is a currency exchange rate display. The traders use this to see the state of the market in real time. It's not rocket science. Reuters provide the difficult stuff. We just subscribe to the appropriate market data and display the numbers as they change. Unfortunately the code was written in a 'learning by emulating' style. Our application is to replace another application and the original developer simply looked at their screen and made sure our numbers did the same things. Unfortunately this style of learning has its limits, it may get you a long way, but eventually the lack of understanding will bite. Last week it did...

The FX code was a mess. It was written in the same style as the rest of the refactoring project; i.e. the code was just spewed into whichever class happened to be open in the editor at the time. Code was duplicated and concepts weren't abstracted. Naming was poor and the code was ugly as hell. We've fixed a lot of that now.

The actual task at hand was to make a couple of simple fixes. Unfortunately, due to the code structure these fixes needed to be done in multiple places and it wasn't obvious where to start. It didn't help that the ticking FX rates were intimately involved with the GUI display. It didn't help that no thought had been given to the data structures involved: Each tick represented a spread of values between a bid and an ask price yet the there was no class to represent a spread and the actions performed on it; traders are interested in the rates pertaining to time spans, called terms, and these can rely on two or more tickers, yet the data structure used was a fixed sized vector of ticker pointers with an enum to index into it, rather than a more obvious structure... All in all working with the code was hard.

We started by trying to understand where to make the fixes. Pair programming with a business analyst (he used to code and managed to catch quite a few typos and mistakes before the compiler did). Eventually we gave up and I spent some time refactoring the code to incorporate a spread class (immediately half of the variables and calculations disappeared), and make the ticker handling a little more explicit. As usual the refactoring involved collecting up all the pieces of code that did stuff with the concept that I was abstracting and putting it all in one place. Lots of files get touched in this kind of work and we spent a couple of hours eyeballing the results to make sure nothing had changed.

If ever there was code that needed to be testable this is it. Unfortunately, though much of the code is now nicely encapsulated it's still far too entwined with the GUI to make testing easy. That's for another day. Tracing code that jumps into action when an external data source changes is hard. Hitting just the right break point isn't easy when the 1month point ticks when you'd hoped that Spot would tick... Bleugh!

After the refactoring lots of patterns emerged in the code. The untouched code is such a mess and so poorly done that similar things have been coded in completely different styles; I guess it depended on his mood. Once a common style was applied (I know, lets just use for loops to iterate over these sequences and lets call the iterator and the variables the same things wherever we use them) the duplicate code began to emerge and was swiftly dealt with.

More pair programming with the business analyst. Once the duplicate code was gone the bugs stood out by being "unbalanced" portions of code. Strange special cases that weren't necessary and once removed the problems went away...

We had one more bug to deal with today; we think we've nailed it.

It was chainsaw refactoring. It's not pretty and you have to trust to luck far more than is healthy, but sometimes it's necessary to be able to move on. We're ready to extract the whole FX rates engine out of the GUI code and into a library, once that's done we can decouple it, mock up its service providers and write the tests... Cool!

Leave a comment