Compromising with legacy code: an anecdote

7 minutes read

Last year I was making a change to some of our legacy code and learned a bit about sacrificing technical correctness for other, less technical, goals in the process.

The code

Here’s the code I wanted to change:

trait ValueStr {
  /** The actual value, e.g. "1/2000 of a second" */
  def raw: String
  /** The actual value, in as little text as possible, e.g. "1/2000s" */
  def rawShort: String
  /** A summary/pretty version of the value, allowed/expected to lose some
      precision, e.g. "1080p" or "Full-frame" */
  def pretty: String
}

(If you don’t know Scala, a trait is like an interface in Java. The defs are the functions that an implementation of this trait must provide; these ones all return String.)

This trait represents a small collection of human-readable strings that describe the value of some property of some thing. (The examples in the comments are properties of cameras — shutter speed, resolution, sensor format — and come from our first website, Snapsort, which compares camera specs.) ValueStr objects are created by code that knows about a particular property and how to talk about its values, and are used by (often very distant) code that knows about the context on the webpage and chooses which description of the value (“raw”, “short”, or “pretty”) would be most appropriate in that context.

What I wanted to do

I wanted descriptions of values to be XML so that I could mark up parts of them to support later processing. For the example in the first comment, I wanted

1/2000 of a second

to become something like

1/<number>2000</number> of a second

(For this post, it doesn’t matter why I wanted to do this, but if you’re curious: the markup is used to analyze translations of this text into other languages, so we can take a specific translation like

1/2000 of a second  →  1/2000 de seconde

and produce a more generic translation like

1/NUMBER of a second  →  1/NUMBER de seconde

The reasons we were trying to do this instead of using more standard internationalization techniques would be a whole other post.)

Attempt 1: Too idealistic

First I considered just changing the types of the return values:

trait ValueStr {
  /** The actual value, e.g. "1/2000 of a second" */
  def raw: NodeSeq
  /** The actual value, in as little text as possible, e.g. "1/2000s" */
  def rawShort: NodeSeq
  /** A summary/pretty version of the value, allowed/expected to lose some
      precision, e.g. "1080p" or "Full-frame" */
  def pretty: NodeSeq
}

(NodeSeq is Scala’s standard data type for representing XML.)

Just changing the types like this is my preferred method for changing code in a statically typed language like Scala. Change the types, let the compiler identify the inconsistencies thus introduced, fix them, go home.

In this case though, that wasn’t going to work. For example, elsewhere in the code we might have something like this:

List("something", "something else", valueStr.raw).mkString(", ")
// before change: "something, something else, 1/2000 of a second"
// after change:  "something, something else, 1/<number>2000</number> of a second"

(Scala’s List.mkString combines strings, inserting a delimiter between them, much like Python’s str.join or Haskell’s intercalate.)

If the string produced by code like this is later inserted into HTML (which is typical for uses of ValueStr), I’ll end up with escaped tags like &lt;number>2000&lt;number> in the webpage, which is a breaking change.

Since there are lots of contexts like this in Scala (as in Java) where arbitrary objects get converted to strings, we can’t rely on the compiler noticing that we’ve made breaking changes to the behaviour.

Attempt 2: Too much mess

So, if I can’t rely on the compiler to identify the points where my changes will break things, I’ll have to look at all the relevant spots myself. To help me find these, I often change the method names too:

import scala.xml.NodeSeq

trait ValueStr {
  /** The actual value, e.g. "1/2000 of a second" */
  def rawNS: NodeSeq
  /** The actual value, in as little text as possible, e.g. "1/2000s" */
  def rawShortNS: NodeSeq
  /** A summary/pretty version of the value, allowed/expected to lose some
      precision, e.g. "1080p" or "Full-frame" */
  def prettyNS: NodeSeq
}

(I lack imagination in names.)

Now the compiler will find errors at all uses of the old raw, rawShort, and pretty methods, and I can look at those uses individually in context to decide whether it’s enough to just call the corresponding new method or decide that additional changes are needed.

(If you’re wondering why I don’t get my IDE to tell me where the methods are used, it’s because I use vim and don’t want to learn new things.)

I gave this approach a good-faith try. What I found was that although the vast majority of uses of ValueStr needed no substantial change, there were obscure corners of our code where the strings returned by the old methods were used in… creative ways. For example: in one spot, the code used the fact that the particular value whose ValueStr appeared there was of a list type, and recovered the list structure from the ValueStr by splitting the .rawShort string on commas. In other spots, I wasn’t able to easily determine exactly what the code was intended to do, or what assumptions it was making about the strings returned by ValueStr. These difficult spots weren’t prevalent, but they were frequent enough to require scrutiny of every use of ValueStr in our codebase.

In a better world, I’d be able to spend some time figuring out what was going on in these derelict areas of the code and develop more righteous ways to do whatever it was they were doing. In this world though, it didn’t make sense for me to spend that time, nor to take the risk of making code changes, especially since many of the problem areas were in products that weren’t going to use the new XML methods anyway, and weren’t business priorities. (We wanted them to keep running, but with minimum further investment.)

Attempt 3: Something for everybody to be unhappy about

What I ended up doing was, first, augmenting ValueStr with new methods for XML:

import scala.xml.{NodeSeq, Text}

trait ValueStr {
  /** The actual value, e.g. "1/2000 of a second" */
  def raw: String
  /** The actual value, in as little text as possible, e.g. "1/2000s" */
  def rawShort: String
  /** A summary/pretty version of the value, allowed/expected to lose some
      precision, e.g. "1080p" or "Full-frame" */
  def pretty: String

  def rawNS: NodeSeq = Text(raw)
  def rawShortNS: NodeSeq = Text(rawShort)
  def prettyNS: NodeSeq = Text(pretty)
}

(The default implementations just wrap the strings in XML text nodes.)

Next, I added a subtrait for instances that have better XML representations, where the String methods extract the text from that XML:

trait ValueNS extends ValueStr {
  final def raw: String = rawNS.text
  final def rawShort: String = rawShortNS.text
  final def pretty: String = prettyNS.text
}

Let’s see how this works out.

  1. Where I need XML representations, I can change the code to create instances of ValueNS, supplying implementations of the *NS methods which return the same text as before, but with markup. If these instances make their way into old code that calls the String-returning methods, that code will see the same values as before the change (because ValueNS.raw and friends use .text, which returns only the textual content of the XML, rather than .toString, which returns a string representation of the markup).

  2. If old code creates instances of ValueStr that make their way into new code where I want XML representations, well, it’ll compile and run, but I’ll probably need to update the old code to get a satisfactory XML representation.

  3. If I’ve updated some code to produce an instance of ValueNS with a nice XML representation, and on its way to the point in the code where the HTML is actually rendered it passes through one of those problem areas that calls the String-returning .raw, does something creative with it, and passes on some new ValueStr… ah, this is a bug that I’ll need to fix.

So, it works out okay. Not great. Not good, even.

In the event, I had to chase down quite a few of these bugs. I was a bit sad to have to do it by testing instead of by static type-checking, but the benefit was huge: I was able to ignore the problem areas in low-priority products.

By letting some of the code be wrong, I had a better chance to make the important parts of the code right. I’m slowly learning how to be okay with that kind of trade-off.

Updated: