Is this a better (more functional way) to write the following fsharp code?

Question

Hi,

I have pieces of code like this in a project and I realize it's not written in a functional way:

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

firmwareVersions is a List which is part of a csharp library. It has (and should not have) any knowledge of how it will be converted into an array of bytes. I realize the code above is very non-functional, so I tried changing 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]

So now I'm wondering if this is a better (more functional) way to write the code. If so... why and what improvements should I make, if not, why not and what should I do instead?

Suggestions, feedback, remarks?

Thank you

Update

Just wanted to add some more information. This is part of some library that handles the data for a binary communication protocol. The only upside I see of the first version of the code is that people implementing the protocol in a different language (which is the case in our situation as well) might get a better idea of how many bytes every part takes up and where exactly they are located in the byte stream... just a remark. (As not everybody understand english, but all our partners can read code)

Answer 1

How often does the code run?

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.

Also, I think perhaps your "Array.reduce(...)" can just be "Array.concat".

Overall I prefer the first way (just create one huge array), though I would factor it differently to make the logic more apparent (e.g. have a named constant HEADER_SIZE, etc.).

While we're at it, I'd probably add some asserts to ensure that e.g. nameBytes has the expected length.

Answer 2

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.

Indexing is normally accidental complexity, in which case it should be avoided. For example, your first version's loop could be for firmwareVersion in firmwareVersion instead of for i = 0 to loops.

Also, like Brian says, using constants for the offsets would make the imperative version even more readable.