Return to Answer

• 1

• 22.8k
• 2
• 16
• 40

##Style

• Comments are just noise and thus reduce readability if they state what is obvious in the code. EG the comment //Read from each line is followed by the line lineReader.eachLine(filePath, (line)=> {

• Do not mix styles. You use semicolons randomly in the code. Use them or not, do not mix them

• Do not add empty blocks or as your comment describes "do nothing" . Example if (line.includes('ATTACK LOG')) { } else should be if (!line.includes('ATTACK LOG')) {

• Do not add magic numbers or string in the code. Define them as constants in one place

• Always delimite statement blocks with {}. EG for(let i of attackData) i && tempAttackData.push(i); should be for(const i of attackData) { i && tempAttackData.push(i) }

• Use constants for variables that do not change

• Single argument arrow functions do not need the () around the argument. EG (line)=> { can be line => {

• Spaces between commas

• Undeclared variable are dangerouse. The variables finalResults, attackData, tempAttackData, cleanedAttackData and removedDateTime are all undecalared and thus are part of the top level scope and may clash creating very hard to debug problems

• Avoid single use variables. EG finalResults = new Results(result); finalResults.save(); can be new Results(result).save();

• i is used as a loop counter and should not be used to represent a value or item of an array. The loop for (let i of ... should be for (const item of...

• Do not add arguments to a function if that argument is not used. EG tempAttackData.forEach((data, index) => You do not use index so there is no need to define it

##Logic

The whole thing feels clumsy as you move data from array to array, variable to variable, and step over and into nested and superfluous statements blocks.

It can be greatly simplified by creating a few helper functions to separate the various tasks. See example.

There also seams to be a bug as you do not check if the line is the second header line (contains "---- LOOT -----").

##Example

The example breaks the task into smaller functions reducing the overall source code (and executional) complexity.

The additional item at the end of the data line is just ignored (rather than popped from the array) by the function Attack which creates the attack object you save.

const LOGS = "/retrieve-logs";
const message = "Results successfully loaded";
const FILE_PATH = "AttackLog-2019-10.log"; 
const FIELDS = "accountNumber,timeOfAttack,trophies,searches,ds,gold,elixir,darkElixir,trophyChange,victory,damagePercentage,bonusGold,bonusElixir,bonusDarkElixir";
const Attack = new Function(...FIELDS.split(","), "return {" + FIELDS + "};");
const valid = line => ! ["ATTACK LOG", "GOLD", "LOOT"].some(mark => line.includes(mark));
const clean = line => line.slice(22).replace("||", "|");
const parse = line => Attack(...clean(line).split("|").map(item => item.trim())); 
router.get(LOGS, (req, res) => { 
 lineReader.eachLine(FILE_PATH, line => { valid(line) && new Results(parse(line)).save() });
 Results.find({}, (err, success) => { res.status(200).send({message, success}) });
});