Problem
I found a challenge on /r/dailyprogrammer that I figured seemed fun to solve: a packet assembler. I solved it, but the feedback on that subreddit is lacking, which is why I turn to you. Each input line is structured like so:
6220 1 10 Because he's the hero Gotham deserves,
Where the first 4 digits is the message ID, the 1 is the index of that packet, the 10 is the total number of packets, and the text string is part of the message. Output should print each packet in correct order and in the order messages are completed. And here’s my solution:
const masterIndex = new Array();
function lineHandler(line) {
let id = Number(line.slice(0, 4));
let index = Number(line.slice(8, 10));
let maxIndex = Number(line.slice(12, 14));
let msg = line.slice(16, line.length);
return [id, index, maxIndex, msg];
}
function pktAssembler(packet) {
//id = 0; index = 1; maxIndex = 2; msg = 3;
//get msgList from masterIndex if it exists:
if (masterIndex[packet[0]]) {
let msgList = masterIndex[packet[0]];
msgList[packet[1]] = packet[3];
//check if msgList is complete:
if (msgList.every(x => Boolean(x))) {
printMsg(packet[0]);
};
} else {
//doesn't exist, build new one.
let msgList = new Array(packet[2]).fill(false);
msgList[packet[1]] = packet[3];
masterIndex[packet[0]] = msgList;
};
}
function builder(str) {
//builds messages
pktAssembler(lineHandler(str));
};
//this is not so pretty; it formats the output
function printMsg(id) {
var msg = masterIndex[id];
var col1 = " ";
for (let i = 0; i < msg.length; i++) {
const line = msg[i];
if (i >= 10) {
var col2 = " ";
} else {
var col2 = " ";
}
if (msg.length >= 10) {
var col3 = col1;
} else {
var col3 = " ";
}
var str = id + col1 + i + col2 + msg.length + col3 + line;
console.log(str);
}
}
//this reads lines
const readline = require('readline');
const fs = require('fs');
const rl = readline.createInterface({
input: fs.createReadStream('js_practice/packet_assembly/packets_assembly.txt'),
crlfDelay: Infinity
});
//this is what's done with each line
rl.on('line', (line) => {
builder(line);
});
Now I turn to you. I don’t know how efficient my code is, but it does get the job done (which is good!). Is there anything I should have done differently? Does pktAssembler
do too much?
Solution
lineHandler
Prefer const
over let
when a variable gets assigned only once.
function lineHandlerOP(line) {
const id = Number(line.slice(0, 4));
const index = Number(line.slice(8, 10));
const maxIndex = Number(line.slice(12, 14));
const msg = line.slice(16, line.length);
return [id, index, maxIndex, msg];
}
Your implementation is very strict. The examples behind the link only show data that fits your format, but what about id
1, would it be formatted as ' 1'
or '0001'
? Or what if additional white space is added between the data?
const line = "6220 1 10 Because he's the hero Gotham deserves,";
// [6220, 1, 10, "Because he's the hero Gotham deserves,"]
If the format changes ever so slightly, results will no longer match.
const line = "6220 1 10 Because he's the hero Gotham deserves,";
// [6220, 1, 1, "Because he's the hero Gotham deserves,"]
Proposed refactoring (initial attempt)
First I figured why not use a string.split
with limit
?
function lineHandler(line) {
return line.split(/s+/, 4).map(
(currentValue, index) => index > 2
? currentValue : Number(currentValue));
}
Unfortunately, in Javascript the remainder is not added to the last element.
const line = "6220 1 10 Because he's the hero Gotham deserves,";
// [6220, 1, 10, "Because"]
Proposed refactoring (using a regex)
Either we need to roll out our own split
method that puts the remainder in the last element or we could use a regex. The regex approach could be written as:
function lineHandler(line) {
const regexp = /(?<id>d+)s*(?<index>d+)s*(?<maxIndex>d+)s*(?<msg>.+)/
const result = regexp.exec(line);
const id = Number(result.groups.id);
const index = Number(result.groups.index);
const maxIndex = Number(result.groups.maxIndex);
const msg = result.groups.msg;
return [id, index, maxIndex, msg];
}
It’s a bit more code, but a lot more resilient to changes in the format.
const line = "6220 1 10 Because he's the hero Gotham deserves,";
// [6220, 1, 10, "Because he's the hero Gotham deserves,"]
const line = "6220 1 10 Because he's the hero Gotham deserves,";
// [6220, 1, 10, "Because he's the hero Gotham deserves,"]