Reading a text file, manipulating it and saving it to a mongo database

Question

We have an upcoming project at work which is going to require working with express.js. I have no prior experience with node.js, so I thought I'd try and do something aside from some courses. Are there any anti-patterns and what can be improved?

Objective

I have a text file that is retrieved from an external source and looks like this (It has a lot more lines than this)

I do not need the first 3 lines, but I do need the data separated by a "|". Wherever there is a "||" it adds an undefined value to the array so I just remove it. I also do not need any text which has a "/" in it.

2019年10月09日 11:03:33 : ============================= ATTACK LOG ==============================
2019年10月09日 11:03:33 : | ------- LOOT ------ ------ BONUS ------
2019年10月09日 11:03:33 : |AC|TIME.|TROP.|SRC|DS| GOLD| ELIXIR| DE|TR.|S| %| GOLD|ELIXIR| DE|L.
2019年10月09日 11:15:44 : 1|11:15| 1761| 17| 4| 450447| 546344|1704|-17|0| 47| 0| 0| 0|--||211/220|
2019年10月09日 11:20:49 : 1|11:20| 1744| 14| 4| 307817| 279805| 643| 10|1| 62| 15640| 15640| 0|G1||209/220|

I am using a library called line-reader to read the text file and my Mongoose schema is defined as Results

 router.get('/retrieve-logs', (req,res,next)=>{
 var headers = ['accountNumber','timeOfAttack',
 'trophies','searches','ds',
 'gold','elixir','darkElixir',
 'trophyChange','victory','damagePercentage',
 'bonusGold','bonusElixir','bonusDarkElixir'];
 const filePath = 'AttackLog-2019-10.log'
 //Read from each line
 lineReader.eachLine(filePath, (line)=> {
 //Do nothing if 'ATTACK LOG' in line
 if (line.includes('ATTACK LOG')) {
 } else {
 //Remove appended date and time
 removedDateTime = line.substring(22);
 if (removedDateTime.includes('GOLD')) {
 } else {
 //Grab lines after headers removed
 attackData = removedDateTime.split("|");
 tempAttackData = [];
 //Remove undefined from array of data and remove last value from array
 for(let i of attackData)
 i && tempAttackData.push(i);
 attackData = tempAttackData;
//Remove last item from array which we don't need 
 attackData.pop();
 // Remove excess whitespace
 cleanedAttackData= [];
 attackData.forEach((data, index) => {
 cleaned = data.trim();
 cleanedAttackData.push(cleaned)
 });
 //Create object for DB {headers: attackData}
 var result = {};
 headers.forEach((headers, i) => result[headers] = cleanedAttackData[i]);
 //Save the created object
 finalResults = new Results(result)
 finalResults.save();
 }
 }
 })
 Results.find({},(err, docs) => {
 res.status(200).send({
 message: "Results successfully loaded",
 success: docs
 })
 })
 })

Answer 1

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}) });
});

• Reez0: Thanks for the help. I'll keep all of this in mind.