HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Download a file and update the current downloaded percentage

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thefileupdateandcurrentdownloadpercentagedownloaded

Problem

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:

  • updateDownloadingPercentage and registerDownloadFinished are networked operations; I should probably be doing something to make sure these things arrive in order



  • 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 sni

Solution

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 ;)

Context

StackExchange Code Review Q#41983, answer score: 2

Revisions (0)

No revisions yet.