Review this code for code quality.
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Data URI encoder</title>
<style>
#dropbox {
padding: 18.5px 0;
max-width: 499px;
border: 2px dashed #bbb;
border-radius: 5px;
color: #bbb;
text-align: center;
}
</style>
</head>
<body>
<h1>Data URI encoder</h1>
<p>Select or drag a file to get the Data URI: <input id="fileinput" type="file"></p>
<div id="dropbox"><h3>Drop file here</h3></div>
<p id="filename"></p>
<p><textarea id="content" rows="6" cols="60" onclick="this.select()"></textarea></p>
<p id="filesize"></p>
<script>
var fileinput = document.getElementById("fileinput"),
dropbox = document.getElementById("dropbox"),
filename = document.getElementById("filename"),
content = document.getElementById("content"),
filesize = document.getElementById("filesize");
function encodeDataURI(e) {
e.stopPropagation();
e.preventDefault();
var files = e.target.files || e.dataTransfer.files,
file = files[0],
reader = new FileReader();
reader.onload = (function() {
return function(e) {
content.value = e.target.result;
filename.textContent = file.name;
filesize.innerHTML = "Data URI size: " + e.target.result.length + " bytes<br>" + "Original size: " + file.size + " bytes"
};
})();
reader.readAsDataURL(file);
}
function handleDragOver(e) {
e.stopPropagation();
e.preventDefault();
e.dataTransfer.dropEffect = "copy";
}
dropbox.addEventListener("dragover", handleDragOver, false);
dropbox.addEventListener("drop", encodeDataURI, false);
fileinput.addEventListener("change", encodeDataURI, false);
</script>
</body>
</html>
2 Answers 2
There isn't much here, just a few comments though:
Most developers I come across use
varper variable rather than comma separated. It's down to personal preference and code habits though, so you could ignore this one.An advantage I see is that it makes variable declaration easier to move around without worrying about the connecting commas.
I don't see the benefit of using a closure for
reader.onload. You can simply just assign it a function.Further, you can move out the
reader.onloadhandler function outsideencodeDataURI. That way, the code won't generate that handler function for every call ofencodeDataURI. Optimizing JS engines will do this for you though, but it's best practice if you just code it that way.Programming languages were designed for people. Name your variables verbosely. For instance,
eintoevent. Short variables are only good in the short-term. Long-term, they aren't good for maintenance. Don't worry about file sizes, that's what minifiers are for.
Your code seems pretty much picture perfect.
- JS beautifying does nothing, I am guessing you used it
- JS hint has nothing to report except a missing semicolon after
filesize.innerHTML = ".. - As for using 1 var, I think that is the right thing to do
- As for your closure in
encodeDataURI, you are accessingfile, so that is justified.
I can only ask that you share this with the world through a github project or github gist.
On a second thought; your code could use some comments, particularly as to which browsers are supported and what tricks you employ to make that work. Especially this line could use a comment as to which browser uses what:
var files = e.target.files || e.dataTransfer.files,
-
\$\begingroup\$ Thanks for your review. I'm a beginner at JavaScript, should I join Github right now? \$\endgroup\$user33979– user339792013年12月26日 08:42:21 +00:00Commented Dec 26, 2013 at 8:42
-
\$\begingroup\$ Interesting, I think you should. \$\endgroup\$konijn– konijn2013年12月26日 20:12:18 +00:00Commented Dec 26, 2013 at 20:12