3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @Raystafarian One named bytesIn32Bits, for instance? What do you think about my argument regarding that 4 appears close to 32 (and the 32 is out of our control), so it's more readable (at least to me, though I did write it, so I'm biased). \$\endgroup\$ – pushkin Apr 19 '18 at 22:55
  • \$\begingroup\$ That's just my opinion. Telling me that 32 bits is 4 bytes isn't a very helpful name. Explaining why you're doing something is better practice than explaining what you're doing. Why is there a 4? What's the cause? What might someone want to know when they see it for the first time? \$\endgroup\$ – Raystafarian Apr 19 '18 at 22:58
  • \$\begingroup\$ @Raystafarian I would generally agree, but my concern with not putting 32 in the name is that people would wonder, "well, we're calling a function that reads 32 bits and then we update the offset by numberOfBytesInLength. I sure hope that's set to 4. Maybe, I'd better check...". The 4 is just how many bytes we decided to use to encode the length of a message. It's not totally arbitrary, but it's also not particularly interesting. \$\endgroup\$ – pushkin Apr 19 '18 at 23:03
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Zeta Apr 20 '18 at 17:07
  • \$\begingroup\$ Node does have a more general function. It is called readIntLE(offset, byteLength) \$\endgroup\$ – Marc Rohloff Apr 20 '18 at 18:29
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good call. I'll create a wrapper Buffer that keeps track of the offset. Thanks \$\endgroup\$ – pushkin Apr 20 '18 at 17:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.