8
\$\begingroup\$

I am not a big fan of JS, but have been forced to use it in this situation for various reasons.

My task: Download a URL to a target file, and update another part of the application with the current percentage.

I've run jslint and jshint over the code, and it seems happy, but I am sure there is still plenty of suboptimal stuff in my code. My code currently looks like this:

Match.prototype.downloadURL = function (url, target, next) {
    winston.info("Downloading url %s to %s", url, target);
    var tmpFilename = target + ".tmp",
        tmpFile = fs.createWriteStream(tmpFilename);
    http.get(url, function (resp) {
        var fullSize = +resp.headers["Content-Length"],
            currentSize = 0,
            currentPercentage = -1;
        // Write the file
        resp.pipe(tmpFile);
        resp.on('end', function () {
            tmpFile.close();
            this.registerDownloadFinished();
            fs.rename(tmpFilename, target, function (err) {
                if (err !== null) {
                    return next(err);
                }
                next(null, target);
            });
        });
        // Update when we hit a new percentage
        resp.on('data', function (chunk) {
            currentSize += chunk.length;
            var percentage = parseInt(currentSize / fullSize * 100, 10);
            if (percentage === currentPercentage) {
                return;
            }
            currentPercentage = percentage;
            this.updateDownloadingPercentage(percentage);
        });
    });
};

Problems I can see:

  1. updateDownloadingPercentage and registerDownloadFinished are networked operations; I should probably be doing something to make sure these things arrive in order
  2. I have a feeling I should be checking for errors a lot more; createWriteStream can fail, so where do I get its error message from.

Again, I only ask because I am a very poor JavaScript coder. Please enlighten me as to any way I can improve this snippet.

\$\endgroup\$
2
\$\begingroup\$

From a once over and reading some docs:

  • You need a resp.on('error', function () {} to deal with http errors
  • You need a tmpFile.on('error', function () {} to deal with fs/stream errors
  • tmpFile is an unfortunate name, i would call it maybe tmpStream or tmpFileStream
  • You use both single and double quotes for strings, stick to one, preferably single quotes
  • resp should be res ( standard in Node ), or response, it bugs me every time I read the code ;)
\$\endgroup\$
  • \$\begingroup\$ Death to single quotes :) Thanks for this, makes sense. Of course, I now have additional questions, but at least my code is correct now. \$\endgroup\$ – BluePeppers Feb 19 '14 at 18:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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