Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What the code does (tested basic, no errors, usecase):

  • Reads all XML files (ending in .yin) from a directory.
  • converts each to JSON
  • store it in another directory, with the same basename as source file
  • ... with .yin replaced by .json

Overt issues:

  • The code has too many nested functions
  • walk module's next() method in multiple places
  • lack of robust error checks and ensuring no uncaught errors left behind

Q. How to refactor the code using Node.js best practices? (things like):

  • flatten out the nesting.
  • avoiding having to remember to call next(), on all if(err) scenarios.
  • does any other (than those included) module improve readability/efficiency?

Two Node.js files, a module and a main file invoking that module, are attached.

File 1: Main:

// main.js
var path = require ('path');
var yin2json = require ('./yin2json');

console.log ("__dirname: " + __dirname);
var templateDir = path.resolve (__dirname, "resources", "mgmtDataTemplates");
var yinDir = path.resolve (templateDir, "yins");
var jsonDir = path.resolve (templateDir, "jsons");

yin2json.convertYins (yinDir, jsonDir);

File 2: Module:

// yin2json.js

    var fs = require('fs');
    var path = require('path');
    var walk = require('walk');
    var xml2js = require('xml2js');

    var jsonDir ;
    var convertYins = function (yin_dir, json_dir) {
        jsonDir =  json_dir;

        var walker = walk.walk(yin_dir, { followLinks: true });
        walker.on("errors", fDirWalkError);
        walker.on("end", fDirWalkEnd);
        walker.on("file", fDirWalkFile);

    }

    function fDirWalkError (err) {
        console.log ("fDirWalkError: " + err);
        next (err);
    }

    function fDirWalkEnd  () {
        console.log ("======= End of directory walk");
    }

    function fDirWalkFile (root, fileStat, next) {
        if (fileStat.name.indexOf(".yin") < 0) {
            console.log ("skipping file " + fileStat.name + " (does not end in .yin)");
            return;
        } else {
            var yin_file = path.resolve(root, fileStat.name);
            console.log ("yin file: " + yin_file);

            fs.readFile(yin_file, function (err, data) {
                if (err) {
                    console.log ("error reading file:" + yin_file);
                    next (err);
                }
                xml2js.parseString (data, function (err, json_obj) {
                    if (err) {
                        console.log (err);
                        next (err);
                    }
                    var json_string = JSON.stringify(json_obj, null, 2);
                    var json_file = path.resolve (jsonDir, path.basename(yin_file).replace(/\.yin$/, ".json"));
                    console.log ("json file: ", json_file);

                    fs.writeFile(json_file, json_string, "utf8", function (err) {
                        if (err) {
                            console.log ("error converting yin (%s) to json(%s)", yin_file, json_file);
                            next (new Error ("error converting yin(" + yin_file + ") to json(" + json_file + ")"));
                        }
                        else {
                            console.log ("Converted yin (%s) to json(%s)", yin_file, json_file);
                        }
                    });
                });
            });
        }
        next ();
    }

module.exports.convertYins = convertYins;
share|improve this question
    
Welcome to Code Review! As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. For examples of good titles, check out Best of Code Review 2014 - Best Question Title Category You may also want to read How to get the best value out of Code Review - Asking Questions. I changed yours, but if you think it's misleading please improve it. – SuperBiasedMan Oct 5 at 10:47
    
@SuperBiasedMan, thanks. that new title works. – user3213604 Oct 5 at 12:52

1 Answer 1

Try using a Promise library like bluebird. it will save you from callback hell and allow you to write your code synchronously.

let Promise = require('bluebird'),
    fs = Promise.promisifyAll(require('fs'))

function readAllFilesInDir(pathArr) {
    // assume i get all files path as array here 
    let allPromises = pathArr.forEach(function (path) {
        return fs.readFileAsync(path, 'utf8')
    })

    return allPromises
}

function doYourThing() {
    // body...
}
readAllFilesInDir(pathArr)
.then(doYourThing)
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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