patternjavascriptMinor
Download a file and update the current downloaded percentage
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:
Problems I can see:
Again, I only ask because I am a very poor JavaScript coder. Please enlighten me as to any way I can improve this sni
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:
updateDownloadingPercentageandregisterDownloadFinishedare 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;
createWriteStreamcan 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
tmpFileis an unfortunate name, i would call it maybetmpStreamortmpFileStream
- You use both single and double quotes for strings, stick to one, preferably single quotes
respshould beres( standard in Node ), orresponse, it bugs me every time I read the code ;)
Context
StackExchange Code Review Q#41983, answer score: 2
Revisions (0)
No revisions yet.