30/10/2009 10:36:47

I don’t know about most developers, but whenever I write code I keep wondering
if I’m writing it the way I should be writing it. I created a question on the subject
over at StackOverflow recently, but I wanted to expand a bit on the subject.

I started out with the following F# code: (Yes I realize I should have used a foreach)

let data = Array.zeroCreate(3 + (int)firmwareVersions.Count * 27)
data.[0] <- 0x09uy                              //drcode
data.[1..2] <- firmwareVersionBytes             //Number of firmware versions

let mutable index = 0
let loops = firmwareVersions.Count - 1
for i = 0 to loops do
    let nameBytes = ASCIIEncoding.ASCII.GetBytes(firmwareVersions.[i].Name)
    let timestampBytes = this.getTimeStampBytes firmwareVersions.[i].Timestamp
    let sizeBytes = BitConverter.GetBytes(firmwareVersions.[i].Size) |> Array.rev

    data.[index + 3 .. index + 10] <- nameBytes
    data.[index + 11 .. index + 24] <- timestampBytes
    data.[index + 25 .. index + 28] <- sizeBytes
    data.[index + 29] <- firmwareVersions.[i].Status
    index <- index + 27

The code above is part of a library which parses binary data from a communication protocol.
”firmwareVersions” is a List of class which is defined in a c# library.
It has no knowledge of how it will be converted into an array of bytes.
I realized the code above is not exactly written in a functional way and figured I’d probably
be burned alive for it so I modified it like this:

let headerData = Array.zeroCreate(3)
headerData.[0] <- 0x09uy
headerData.[1..2] <- firmwareVersionBytes

let getFirmwareVersionBytes (firmware : FirmwareVersion) =
    let nameBytes = ASCIIEncoding.ASCII.GetBytes(firmware.Name)
    let timestampBytes = this.getTimeStampBytes firmware.Timestamp
    let sizeBytes = BitConverter.GetBytes(firmware.Size) |> Array.rev
    Array.concat [nameBytes; timestampBytes; sizeBytes]

let data = 
    firmwareVersions.ToArray()
    |> Array.map (fun f -> getFirmwareVersionBytes f)
    |> Array.reduce (fun acc b -> Array.concat [acc; b])

let fullData = Array.concat [headerData;data]

The implementation of the protocol handler serves as an example to other developers who
will be implementing the same protocol in different languages (hardware developers) and those
developers hardly understand English so the protocol specification document is pretty useless
to them. Keeping this in mind I thought (and still do) that the first implementation would
give them a much better idea of how to parse the data. The exact positioning of the bytes
is very clear in the first version while in the second that information is abstracted away.

To my surprise the people who responded seemed to agree and disagree with each other:

I like your first version better because the indexing gives a better picture of the offsets, which are an important piece of the problem (I assume). The imperative code features the byte offsets prominently, which might be important if your partners can't/don't read the documentation. The functional code emphasises sticking together structures, which would be OK if the byte offsets are not important enough to be mentioned in the documentation either.

(By Nathan Sanders)

The advantage of 'array concatenation' is that it does make it easier to 'see' the logical portions. The disadvantage is that it creates a lot of garbage (allocating temporary arrays) and may also be slower if used in a tight loop.

(By Brain)

These responses made me wonder, what do I actually gain from writing this code
in a more functional way? The details are less obvious to the other developers and
I might get a decent amount of memory overhead. As I understand it, in the second
version I care more about what I want to do then how I want to do it, but given the
fact that the code also serves as documentation in a “universal” language I still prefer
the first “imperative” version of the code. (But maybe I’m missing the point somewhere)

To add to my own confusion (and loss of confidence) I was going over some old code:

public void DefineClockFields(byte address, List<ClockField> clockFields)
{
    var data = new byte[clockFields.Count * 19 + 3];
    data[0] = 0x0D;
    data.InsertByteArray(BitConverter.GetBytes((ushort)clockFields.Count).Invert(), 1);
    var position = 3;

    foreach (var field in clockFields)
    {
        var definitionData = dataFactory.CreateDefinitionData(field);
        data.InsertByteArray(definitionData, position);
        position += definitionData.Length;
    }

    SendData(data, DEFAULT_MESSAGEGROUP_VERSION, address, 701, 74, AckResponseProcessor);
}

This code is part of a prototype test client I used to test the hardware emulator for the
protocol. Not the most memory efficient code as you can see. Slapping myself in the head
(Although I was suffering from jetlag and other things when I wrote this) I refactored the
sources and the above method was translated into this:

public void DefineClockFields(byte address, List<ClockField> clockFields)
{
    using (var stream = new MemoryStream(clockFields.Count * 19 + 3))
    using (var writer = new BinaryWriter(stream))
    {
        writer.Write((byte)0x0D)
        writer.Write(BitConverter.GetBytes((ushort)clockFields.Count).Invert());
        
        foreach(var field in clockFields)
        {
            var definitionData = dataFactory.CreateDefinitionData(field);
            writer.Write(definitionData);
        }

        SendData(stream.ToArray(), DEFAULT_MESSAGEGROUP_VERSION, 
                 address, 701, 74, AckResponseProcessor);
    }
}

It’s certainly makes more efficient use of memory and it’s still quite readable.
I don’t feel any relevant information was lost either. I decided to go functional crazy:

public void DefineClockFields(byte address, List<ClockField> clockFields)
{
    using (var stream = new MemoryStream(clockFields.Count * 19 + 3))
    using (var writer = new BinaryWriter(stream))
    {
        writer.Write((byte)0x0D)
        writer.Write(BitConverter.GetBytes((ushort)clockFields.Count).Invert());
        
        clockFields.ForEach(f => writer.Write(dataFactory.CreateDefinitionData(f));

        SendData(stream.ToArray(), DEFAULT_MESSAGEGROUP_VERSION, 
                 address, 701, 74, AckResponseProcessor);
    }
}

There you go, even less code… but at what cost? Is it still readable?
I’m really wondering what others think of all of this… how far should we take this?

I’d really like some feedback, what would you do, what would you avoid, etc…