CodeSOD: Going Tubing

Emma X maintains some software for a company that does mechanical engineering. In this specific case, her software needs to match a part on their equipment to a tube which can hold the part. They're operating on the order of millimeters, so the smallest tube is about 8mm, and the largest is 155mm.

Now, obviously, not every possible millimeter size is possible- from 8mm to 58mm, the possible diameters all proceed at a step of 2mm. Between 60mm and 95mm, the step is 5mm, and between 100 and 150, the step is 10mm. As I describe this problem, you're probably imagining a lookup table of some kind, or maybe just a list of possible diameters generated by for loops or some similar construct. If that were how this worked, the code wouldn't be there.

fun tubeDiameter(partDiameter: Length): Length { val partDiameterWithBuffer = partDiameter + 0.001.toBigDecimal() val res = when { partDiameterWithBuffer.res < 0.008.toBigDecimal() -> 0.008.toBigDecimal() partDiameterWithBuffer.res < 0.06.toBigDecimal() -> { val mod = partDiameterWithBuffer.res.rem(0.002.toBigDecimal()) partDiameterWithBuffer.res + mod } partDiameterWithBuffer.res < 0.1.toBigDecimal() -> { val mod = partDiameterWithBuffer.res.rem(0.005.toBigDecimal()) partDiameterWithBuffer.res + mod } partDiameterWithBuffer.res < 0.155.toBigDecimal() -> { val mod = partDiameterWithBuffer.res.rem(0.01.toBigDecimal()) partDiameterWithBuffer.res + mod } else -> { 0.155.toBigDecimal() } } return Length(res, LengthUnits.METER)}

This is Kotlin, which I'm not extremely familiar with, but is easy to read.

The first step is that they add a small buffer to the part diameter, representing their tolerances. Then we have what is a series of conditions. If the diameter is less than 0.008, then we return 0.008. If it's less than 0.06, we take mod 0.002- 2mm- and add that to the diameter. Then if it's less than 0.1, we take the 5 millimeter step, if it's less 0.155, we take the 10mm step.

Logically, this seems to make sense, and seems to match to our problem, so what's the WTF? Well, there are a lot of issues.

First, and this is just convenience, we're working with parts in the scale of millimeters and in this function, we're doing our units in meters. That's just annoying and hurts readability. But the WTF starts when we note that the Length class includes in<Unit> functions, like inMeters, and inMillimeters. res just returns the raw underlying value it was created in, which frequently is meters, but isn't guaranteed to be.

The real problem, though, is that this simplified logic seems like it maps to our domain, but it doesn't actually do it correctly.

Take a partDiameterWithBuffer that is 0.07m. That goes down the 10mm branch, and matches it to 0.07m, which is a perfect fit for our tolerance. Everything is good. But what happens when we feed it 0.071m? Well, the remainder of 0.071m and 0.005m is... 0.001m. So it suggest a tube diameter of 72mm, which is not a valid tube diameter. The code is just wrong.

And how does wrong code like this make it into production? Sing along everybody: because there are no unit tests.

The only positive thing in this code is that they consistently use BigDecimal, which ensures no floating point rounding errors.

Now, Emma has fixed this, and she has "fixed" this. The first fix was just generating a list of valid values using some Kotlin syntactic sugar to make it easy to write:

companion object { val defaultTubeDiameters = ( (8..58 step 2) + (60..95 step 5) + (100..150 step 10) + 155 ).map { Length(it.toBigDecimal(), LengthUnits.MILLIMETER) }}

The second "fix" was- well, I'll let Emma explain:

I have also been somewhat new to Kotlin and highly motivated and replaced the implementation of fun tubeDiameter with an overly complicated binary search on defaultTubeDiameters, but that is for my successor to complain about :) At least it works as expected (more often than before).

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!