CodeSOD: A Bit of a Confession

Today, John sends us a confession. This is his code, which was built to handle ISO 8583 messages. As we'll see from some later comments, John knows this is bad.

The ISO 8583 format is used mostly in financial transaction processing, frequently to talk to ATMs, but is likely to show up somewhere in any transaction you do that isn't pure cash.

One of the things the format can support is bitmaps- not the image format, but the "stuff flags into an integer" format. John wrote his own version of this, in C#. It's a long class, so I'm just going to focus on the highlights.

private readonly bool[] bits;

Look, we don't start great. This isn't an absolute mistake, but if you're working on a data structure that is meant to be manipulated via bitwise operations, just lean into it. And yes, if endianness is an issue, you'll need to think a little harder- but you need to think about that anyway. Use clear method names and documentation to make it readable.

In this developer's defense, the bitmap's max size is 128 bits, which doesn't have a native integral type in C#, but a pair of 64-bits would be easier to understand, at least for me. Maybe I've just been infected by low-level programming brainworms. Fine, we're using an array.

Now, one thing that's important, is that we're using this bitmap to represent multiple things.

public bool IsExtendedBitmap{get{return this.IsFieldSet(1);}}

Note how the 1st bit in this bitmap is the IsExtendedBitmap flag. This controls the length of the total bitmap.

Which, as an aside, they're using IsFieldSet because zero-based indexes are too hard:

public bool IsFieldSet(int field){return this.bits[field - 1];}

But things do get worse.

/// <summary>/// Sets a field/// </summary>/// <param name="field">/// Field to set /// </param>/// <param name="on">/// Whether or not the field is on /// </param>public void SetField(int field, bool on){this.bits[field - 1] = on;this.bits[0] = false;for (var i = 64; i <= 127; i++){if (this.bits[i]){this.bits[0] = true;break;}}}

I included the comments here because I want to highlight how useless they are. The first line makes sense. Then we set the first bit to false. Which, um, was the IsExtendedBitmap flag. Why? I don't know. Then we iterate across the back half of the bitmap and if there's anything true in there, we set that first bit back to true.

Which, by writing that last paragraph, I've figured out what it's doing: it autodetects whether you're using the higher order bits, and sets the IsExtendedBitmap as appropriate. I'm not sure this is actually correct behavior- what happens if I want to set a higher order bit explicitly to 0?- but I haven't read the spec, so we'll let it slide.

public virtual byte[] ToMsg(){var lengthOfBitmap = this.IsExtendedBitmap ? 16 : 8;var data = new byte[lengthOfBitmap];for (var i = 0; i < lengthOfBitmap; i++){for (var j = 0; j < 8; j++){if (this.bits[i * 8 + j]){data[i] = (byte)(data[i] | (128 / (int)Math.Pow(2, j)));}}}if (this.formatter is BinaryFormatter){return data;}IFormatter binaryFormatter = new BinaryFormatter();var bitmapString = binaryFormatter.GetString(data);return this.formatter.GetBytes(bitmapString);}

Here's our serialization method. Note how here, the length of the bitmap is either 8 or 16, while previously we were checking all the bits from 64 up to see if it was extended. At first glance, this seemed wrong, but then I realized- data is a byte[]- so 16 bytes is indeed 128 bits.

This gives them the challenging problem of addressing individual bits within this data structure, and they clearly don't know how bitwise operations work, so we get the lovely Math.Pow(2, j) in there.

Ugly, for sure. Unclear, definitely. Which only gets worse when we start unpacking.

public int Unpack(byte[] msg, int offset){// This is a horribly nasty way of doing the bitmaps, but it works// I think...var lengthOfBitmap = this.formatter.GetPackedLength(16);if (this.formatter is BinaryFormatter){if (msg[offset] >= 128){lengthOfBitmap += 8;}}else{if (msg[offset] >= 0x38){lengthOfBitmap += 16;}}var bitmapData = new byte[lengthOfBitmap];Array.Copy(msg, offset, bitmapData, 0, lengthOfBitmap);if (!(this.formatter is BinaryFormatter)){IFormatter binaryFormatter = new BinaryFormatter();var value = this.formatter.GetString(bitmapData);bitmapData = binaryFormatter.GetBytes(value);}// Good luck understanding this. There be dragons belowfor (var j = 0; j < 8; j++){this.bits[i * 8 + j] = (bitmapData[i] & (128 / (int)Math.Pow(2, j))) > 0;}return offset + lengthOfBitmap;}

Here, we get our real highlights: the comments. "... but it works... I think...". "Good luck understanding this. There be dragons below."

Now, John wrote this code some time ago. And the thing that I get, when reading this code, is that John was likely somewhat green, and didn't fully understand the problem in front of him or the tools at his disposal to solve it. Further, this was John's independent project, which he was doing to solve a very specific problem- so while the code has problems, I wouldn't heap up too much blame on John for it.

Which, like many other confessional Code Samples-of-the-Day, I'm sharing this because I think it's an interesting learning experience. It's less a "WTF!" and more a, "Oh, man, I see that things went really wrong for you." We all make mistakes, and we all write terrible code from time to time. Credit to John for sharing this mistake.

[Advertisement] Otter - Provision your servers automatically without ever needing to log-in to a command prompt. Get started today!