CodeSOD: Consultant Conversions

Janet‘s company had a glut of work, and thus didn’t have the staffing required to do it all. It didn’t make sense to hire on any new full-time employees, so they went the route of bringing on a few highly paid consultants, specifically ones who specialized in one specific problem: talking to a piece of hardware purchased from a vendor.

The hardware in question was a scientific which communicated over a serial line. This device provided a lot of data that represented decimal values, but that data was not encoded as an IEEE float. Instead, they used two integers- one for the data, and one representing the number of decimal places.

So, for example, “555.55” would be represented as “55555 2”.

Now, in embedded devices, this isn’t too unusual. It’s entirely possible that the embedded CPU didn’t even support true floating point operations, and this was just how they decided to work around that.

When communicating over the serial line, the device didn’t send the data encoded in binary, however- it did everything as text. This was arguably helpful as it meant a technician could communicate with the device directly over a terminal emulator, but it meant any software talking to the device had to parse data.

Which brings us to the code written by the highly paid consultants. This code needs to take two 16-bit integers and turn them into a single decimal value. Let’s see how they did it.

/// <summary>
/// Sets the single parameter value.
/// </summary>
/// <param name="Value">Name of the parameter.</param>
/// <param name="decimals"></param>
/// <returns></returns>
public double ConvertIntToDecimal(string Value, string decimalCount)
{
    double Result;

    var decimals = UInt16.Parse(decimalCount);
    var Val = UInt16.Parse(Value);
    if (decimals > 0)
    {
        var divider = Math.Pow(10, decimals);
        Result = ((float)Val) / divider;
    }
    else
    {
        Result = Val;
    }

    return Result;
}

We start with comments that are just wrong, which is always a good start. The whole thing has delightfully randomized capitalization- a mix of PascalCase and camelCase.

In the core logic, we parse the input values, and if there are any decimal places, we do some arithmetic to build our floating point value. We get the fun bonus inconsistency of casting to float when we handle our result in double, but at least it’s a widening inconsistency, I suppose.

As an overall approach to the problem, it’s not a train wreck, but there’s one very important thing that our highly paid consultant forgot. Our HPC, remember, was an expert in this particular instrument, or at least that was their claim. And while their mistake is an easy mistake to make while coding, it should also be an easy mistake to catch during testing, too.

What was the mistake?

The value is frequently negative, and they’re using UInt16 to parse the inputs. Which means this function frequently threw an exception. Literally five minutes of testing would have turned it up. Janet had piles of sample data, recorded from the device, which she used for testing. Almost all of her test cases would trigger the bug at some point.

It seems likely, at this juncture, that the HPC simply never actually tested the code. They wrote it. They committed it. They collected their check and left. Janet may have been the first person to actually run the code at all.

In the end, hiring the HPC cost a lot of money, and maybe saved a few days of work over the course of months. It’s hard to say, as it may have created more work, since so much of what the HPC did had to be debugged and often rewritten.

The “good” news is that they have another glut of work, so management is looking to bring back the consultants for another round.

Source: Read More