Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I want to read the content of file then want to make few updates to content and then write that content to another file. I have created the code with node.js as follows.

var filePath = path.join('public/index.html'),
    exactFileName = path.basename(filePath),
    outputDir = "output/";
function readContent(callback) {
    //start reading file
    fs.readFile(filePath, {encoding: 'utf-8'}, function (err, content) {
        if (err) return callback(err)
        callback(null, content);

        //Check if directory available or not, if no then create new directory
        if(!fs.existsSync(outputDir)){
            fs.mkdirSync(outputDir, 0766, function(err){
                if(err){ 
                    console.log(err);
                    response.send("ERROR! Can't make the directory! \n");    
                }
            });   
        }

        /*
            will process content here and then write to output file
        */

        //write the data in another file        
        fs.writeFile(outputDir+''+exactFileName, content, function (err) {
            if (err) return console.log(err);
            console.log('Content written to '+ exactFileName);
        });        
    })
}

readContent(function (err, content) {
    console.log(content);
});

I need inputs on:

  1. optimizing this code
  2. direction on how we can apply it for multiple files
share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

Interesting question,

there is really not much to optimize from a speed perspective. From a code organization perspective you should really investigate libraries that help with asynchronous coding.

From a pure coding perspective, fs.mkdirSyn will ignore the callback function you provide.

From a reliability perspective, your code will try to write to the output folder if that folder could not be created. That is not good. Also where would response be defined in that callback function?

You can apply this to multiple files by providing filePath as a parameter to readContent. readContent is by the way a terrible function name, it misleads the reader in thinking it just reads content whereas it reads contents, checks directories, optionally creates directories, processes content and writes out content. You get my point..

I am also not sure why you call the callback in readContent before all the work is done, an error might occur in creating a directory or in writing out the new content and the callback would never know..

Update: This code works and organizes the functionality into steps without using a 3rd party library for handling asynchronous calls:

var path = require('path'),
    fs   = require('fs');
    outputDir = "output/";

function readFile( path, callback )
{
  fs.readFile( path, {encoding: 'utf-8'}, callback );
}

function verifyFolder( folderName , callback )
{
  if( fs.existsSync(outputDir) )
    return callback(true);
  fs.mkdir( outputDir, 0766, function(err){
    callback( err ? false : true );
  });
}

function processContent( content , callback ){
  callback( undefined, content.toUpperCase() );
}

function writeContent( path, content , callback )
{
  fs.writeFile( path, content, function (err) {
    if (!err)
      console.log( 'Content written to '+ path );
    callback( err, content );
  });  
}

function processFile( filePath, callback) {

  var content; 

  readFile( filePath, readFileCallback );

  function readFileCallback( err, data ){
    if(err)
      return callback( err, data );
    content = data;
    verifyFolder( outputDir , verifyFolderCallback );
  }

  function verifyFolderCallback( exists ){
    exists ? processContent( content , processContentCallback ) : callback( outputDir + ' does not exist' );
  }

  function processContentCallback( err, content )
  {
    if(err)
      return callback( err, content );
    writeContent( outputDir + path.basename(filePath) , content, callback );    
  }
}

function logResult( err, result ){
  console.log( err || result );
}

processFile( path.join('killme.js') , logResult );
share|improve this answer
    
Agreed on all your points and right now started working on points. Thanks for your inputs. –  Harshal Sawant Jun 26 at 18:45
add comment

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.