CODEWITHSUNDEEP
javascriptnode.jspathfs
gazzwi86
gazzwi86

Reputation: 1030

Renaming files fails when 2 are added simultaneously

The script below is working fantastically when monitoring a folder for new .ogg files. It successfully creates a folder with the new filename and then renames the file according to the file according to its created date/

However, the issues arise when I add multiple files at the same time as the script attempts to create a folder that already exists, suggesting it is mixing up the two filenames somehow. Has anyone any suggestions as to what I might be doing incorrectly? I presume its simple code structure although I'm not able to work out why.

var baseDir = './',
    path = require('path'),
    fs = require('fs');

// watch the directory for new files
fs.watch(baseDir,  function(event, file) {

    var ext = path.extname(file)
        basename = path.basename(file).substring(0, path.basename(file).length - ext.length);

    // check it wasnt a delete action
    fs.exists(baseDir + file, function(exists) {

        // check we have the right file type
        if(exists && ext === '.ogg'){

            // get the created date
            fs.stat(baseDir + file, function (err, stats){

                if (err)
                    throw err;

                var year = stats.ctime.getFullYear();
                var month = stats.ctime.getMonth()+1;
                var day = stats.ctime.getDate();
                var hour = stats.ctime.getHours();
                var sec = stats.ctime.getSeconds();

                if(month < 10){
                    month = '0' + month;
                }
                if(day < 10){
                    day = '0' + day;
                }
                if(hour < 10){
                    hour = '0' + hour;
                }
                if(sec < 10){
                    sec = '0' + sec;
                }

                var name = year + '' + month + '' + day + '' + hour + '' + sec;

                // does the basename directory exist?
                fs.exists(baseDir + '/' + basename, function(exists) {

                    // if the directory doesnt exist
                    if(!exists){

                        // make the directory
                        fs.mkdir(baseDir + '/' + basename, 0777, function (err, stats){

                            if (err)
                                throw err;

                            moveFile(file, basename, name, ext);

                        });

                    } else {

                        moveFile(file, basename, name, ext);

                    }

                });

            });

        }

    });

});

function moveFile(file, basename, name, ext){

    // move the file to the new directory
    fs.rename(baseDir + file, baseDir + '/' + basename + '/' + name + ext, function (err) {

        if (err)
            throw err;

        // console.log('Rename complete');

    });

}

Upvotes: 0

Views: 206

Answers (1)

dwerner
dwerner

Reputation: 6602

Ok, so I had a few extra minutes and decided to have a look for you. I refactored your code a little, but the basic structure should be easy to recognize.

var baseDir = './test',
    path = require('path'),
    fs = require('fs');

// watch the directory for new files
fs.watch(baseDir,  function(event, file) {
  var ext = path.extname(file),
  basename = path.basename(file).substring(0, path.basename(file).length - ext.length);
  // check it wasnt a delete action
  // check we have the right file type
  var filePath = path.join(baseDir, file);
  if(fs.existsSync(filePath) && ext === '.ogg'){
    // get the created date
    var stats = fs.statSync(filePath);
    var name = getName(stats);
    // if the directory doesnt exist
    var baseDirPath = path.join(baseDir, basename);
    if(!fs.existsSync(baseDirPath)){
      // make the directory
      fs.mkdirSync(baseDirPath, 0777);
    }
    moveFile(file, basename, name, ext);
  }
});


function getName (stats) {
  var year = stats.ctime.getFullYear();
  var month = stats.ctime.getMonth()+1;
  var day = stats.ctime.getDate();
  var hour = stats.ctime.getHours();
  // need minutes!
  var minutes = stats.ctime.getMinutes();
  var sec = stats.ctime.getSeconds();
  if(month %lt 10){
    month = '0' + month;
  }
  if(day < 10){
    day = '0' + day;
  }
  if(hour < 10){
    hour = '0' + hour;
  }
  if(minutes < 10){
    minutes = '0' + minutes;
  }
  if(sec < 10){
    sec = '0' + sec;
  }

  // missing the minute, previously
  return year + '' + month + '' + day + '' + hour + '' + minutes + '' + sec;
}

function moveFile(file, basename, name, ext){
  // move the file to the new directory
  var src = path.join(baseDir, file),
      dest = path.join(baseDir, basename, name+ext);

  console.log("Moving ", src, "-", dest);

  fs.renameSync(src, dest);
}

Some tips/corrections:

  1. Stick with the synchronous fs methods that end in Sync when working on simple scripts like this. While node.js is famous for it's asynchronous ability, it's a bit of a premature optimization IMO. If you need to embed this in a high-performance webserver, for instance, optimize at that point, not before.

  2. You were missing a minutes variable when you create the new filename. This has a pretty good chance of causing a name collision, so I corrected it.

  3. Try to use the path library (like path.join) more to your advantage, as manually joining strings for paths can often lead to brittle code.

  4. There are still several edge cases where this can crash. Creating a file without an extension that will have the same name as a directory you will create based on another file. (Files can't become directories, and you can't move a file inside another file.). If you plan to go into a production environment, you will want to harden the code with at least a few unit tests.

Cheers,

Dan

Upvotes: 1

Related Questions