Import XML files to MongoDb with Nodejs

Question

Here is my code which will read XML files and import the data to MongoDb. Do you have any comments on my code?

const fs = require('fs');
const xml2js = require('xml2js');
const log = console.log;
const path = require( "path" );
const folderPath = 'D:/Data';
const folder = folderPath;
fs.readdirSync(folder).forEach(file => {
 const extname = path.extname(file);
 const filename = path.basename(file, extname);
 const absolutePath = path.resolve(folder, file);
 const parser = new xml2js.Parser({
 mergeAttrs: true,
 explicitArray: false
 });
 fs.readFile(absolutePath, (err, data) => {
 parser.parseString(data, (err, result) => {
 var MongoClient = require('mongodb').MongoClient;
 var url = "mongodb://localhost:27017/";
 MongoClient.connect(url, function (err, db) {
 if (err) throw err;
 var dbo = db.db("myDb");
 dbo.collection("entity").insertMany(result, function (err, res) {
 if (err) throw err;
 db.close();
 });
 });
 });
 });
});

I tested that it works for few files but for hundreds of files, it can throw an error.

• silicakes: What's the error it throws?
• Steve Ngai: Connection time out. Should I post it to stack overflow instead?
• silicakes: Yeah sure, I think your issue is with the fact you do your reading synchronously, thus blocking your main thread (and killing the process due to a timeout). Try running it asynchronously first.

Answer 1

Variable declaration keywords

One thing I notice is that the var keyword is used in some functions while others use const. The variables dbo, MongoClient and url are never reassigned so const can be used for those. Using const when re-assignment isn't needed is a good habit because it can help avoid accidental re-assignment and other bugs. Many users also believe var should not be used with ES6 code - refer to the ES lint rule no-var.

nested callbacks

The code is bordering on callback hell. I'd suggest moving anonymous functions out to being named functions. This will also help with automated testing. Let's look at the inner most function:

const insertionCallback = (db, err, res) {
 if (err) throw err;
 db.close();
};

It needs a reference to db. That could be made a parameter:

function (db, err, res) {

Then when making a reference to that function, make a partially applied function with db as the first argument:

dbo.collection("entity").insertMany(result, insertionCallback.bind(null, db))

Or instead of accepting db as a parameter, set the context to it:

dbo.collection("entity").insertMany(result, insertionCallback.bind(db))

after doing that this could be used instead of db within that insertionCallback function.

Then continue pulling out those nested functions - e.g. :

const insertResult = (result, err, db) => {
 if (err) throw err;
 const dbo = db.db("myDb");
 dbo.collection("entity").insertMany(result, insertionCallback.bind(null, db));
};

Then that function can be used in its outer function:

const handleParsedStr = (err, result) => {
 const MongoClient = require('mongodb').MongoClient;
 const url = "mongodb://localhost:27017/";
 MongoClient.connect(url, insertResult.bind(null, result));
});

And that functions can be used as the callback to parser.parseString():

parser.parseString(data, handleParsedStr);

error handling

Some of the callback functions check error arguments like err and conditionally handle such cases, but others don't- e.g. the callback to parser.parseString(). It is advisable to handle such errors, and would also make the code more consistent.

parser declared within forEach callback function

The variable parser is declared within the callback function to the .forEach() method. This means the parser is created once for each file in the directory. If possible, consider declaring it outside the loop to reduce resource requirements.

• Steve Ngai: Thanks a lot for the comprehensive review. Learned much here :)