Updating an offset in a buffer after each read

Question

I'm having a bit of a debate at work regarding when to replace numeric literals with variable names.

I have a function that reads data from a buffer and after each read, updates the offset so that next time, we read starting at the right offset.

function readMessage(data: Buffer, offset: number) {
    let firstLen = data.readInt32LE(offset);
    offset += 4;

    let firstPieceOfMessage = data.slice(offset, offset + firstLen).toString();
    offset += firstLen;

    let secondLen = data.readInt32LE(offset);
    offset += 4;

    let secondPieceOfMessage = data.slice(offset, offset + secondLen).toString();
    offset += secondLen;

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return [finalMessage, offset];
}

The calling code calls this function in a loop:

let offset = 0;
while (offset < data.length) {
    let message;
    ([message, offset] = readMessage(data, offset));
    console.log(message);
}

A coworker made a comment that I should replace the 4 with a constant defined elsewhere.

My view is that since 4 is intimately-bound to the function readInt32LE, and the offset updating happens immediately after reading the 32 bits, it's more readable to keep the 4 and 32 next to each other.

Replacing 4 with a variable like numberOfBytesPerMessagePiece would jump out at me, because then I'll wonder "Wait, this would only work if that variable is set to 4, because that aligns with the function that I'm using above it. Is it actually set to that?"

If Node had a more general function readIntLE(offset, 4), then I would gladly replace 4 with a constant, but since there isn't, and we must use a function whose name contains a literal, it feels better to keep the literal 4 next to it.

Second, changing the constant to something else would necessarily break the code, since one would have to link that change with replacing readInt32LE with something more apt.

---

This is complicated by the fact that we use 4 in other places in this file (but not in other files), but almost all places (all but one) use it next to a similar function:

let buff = Buffer.alloc(4 + headerLen);
buff.writeInt32LE(someLength, 0);
buff.write(header, 4);

---

I'm thinking now that I could call this constant bytesIn32Bits = 4;. Is this worth the trouble?

---

Update:

This is one option I'm exploring right now:

function readLength(data: Buffer, offset: number): [number, number] {
    const len: number = data.readInt32LE(offset);
    offset += 4;

    return [len, offset];
}

function readData(data: Buffer, offset: number, length: number): [Buffer, number] {
    let msg: Buffer = data.slice(offset, offset + length);
    offset += length;

    return [msg, offset];
}

This will at least allow me to get rid of the offset updates in the first example.

function readMessage(data: Buffer, offset: number) {
    let firstLen: number;
    ([firstLen, offset] = readLength(data, offset));

    let firstPieceOfMessage: string;
    ([firstPieceOfMessage, offset] = readData(data, offset, firstLen));

    let secondLen: number;
    ([secondLen, offset] = readLength(data, offset));

    let secondPieceOfMessage : string;
    ([secondPieceOfMessage, offset] = readData(data, offset, secondLen));

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return [finalMessage, offset];
}

I'm not convinced that that looks any nicer though.

Answer 1

I tend to agree with your colleague - magic numbers are nearly always bad. In this case, it is clear why you are adding 4, but only once I've done 32/8 in my head.

Extracting this constant into a bytesIn32Bits would certainly be helpful for me and other developers who don't work on the byte level very often.

However, I don't believe this is the ideal solution. Though the code itself isn't really that repetitive, the logic is.

1. Read data
2. Advance offset by length of data

Unfortunately Node does not have a built in Buffer alternative which can keep track of an offset for you. You could do this with functions as shown in the edit, but it would be better to handle at the class level as you can then completely forget about the offset in most cases.

Ideally, the function should read something like this:

function readMessage(data: OffsetTrackingBuffer) {
    let firstLen = data.readInt32LE();
    let firstPieceOfMessage = data.readString(firstLen);
    let secondLen = data.readInt32LE();
    let secondPieceOfMessage = readString(secondLen);

    let finalMessage = firstPieceOfMessage + secondPieceOfMessage;
    return finalMessage;
}

The outer loop would also be slightly changed:

let data = new OffsetTrackingBuffer(rawData, 0);
while (data.offset < data.length) {
    let message = readMessage(data);
    console.log(message);
}

I'm not normally one to recommend adding libraries to your project, but in this case, all this logic has already been implemented in the smart-buffer npm package. According to the readme it also comes with TypeScript typings built in. I haven't used it, but it seems to be rather popular.