Back in high school, I read a brilliant book, DOM Scripting (first edition) by Jeremy Keith. The Chinese translation of the book had a even better title: "JavaScript DOM 编程艺术", which literally means "the art of JavaScript DOM programming". The book was the first book I bought about JavaScript. It was so enlightening that it showed me how to write JavaScript in a "good" way (graceful degradation, in particular), and it eventually turned me into a frontend developer a decade later. If you ask to me to recommend 3 books for learning JavaScript, I'll definitely have DOM Scripting in the list.
Last year, I saw a JavaScript function appeared in a question on SegmentFault, where the post author asked for some explanation on the code he posted. I wrote an answer:
The code you posted is a piece of shit. You can learn nothing from it. Actually, I've never seen any JavaScript code worse than that.
The post author replied: "I read the code from Keith's DOM Scripting". I was astonished. Recalling that DOM Scripting is such a masterpiece, I cannot believe it. After realizing the code was really extracted from the book, I felt embarrassed and woke up to the fact that JavaScript had really changed a lot over the years.
Therefore, I made it a interview question for frontend developer candidates: read the very code section, and tell me how will you improve it. Only include basic code issues (including code style issues). Don't change the logic. Don't use fancy ES5/6 things. Don't say "requestAnimationFrame API" can help the performance. Just focus on the code itself. Here's the original code, again:
function moveElement(elementID,final_x,final_y,interval) {
if (!document.getElementById) return false;
if (!document.getElementById(elementID)) return false;
var elem = document.getElementById(elementID);
if (elem.movement) {
clearTimeout(elem.movement);
}
if (!elem.style.left) {
elem.style.left = "0px";
}
if (!elem.style.top) {
elem.style.top = "0px";
}
var xpos = parseInt(elem.style.left);
var ypos = parseInt(elem.style.top);
var dist = 0;
if (xpos == final_x && ypos == final_y) {
return true;
}
if (xpos < final_x) {
var dist = Math.ceil((final_x - xpos)/10);
xpos = xpos + dist;
}
if (xpos > final_x) {
var dist = Math.ceil((xpos - final_x)/10);
xpos = xpos - dist;
}
if (ypos < final_y) {
var dist = Math.ceil((final_y - ypos)/10);
ypos = ypos + dist;
}
if (ypos > final_y) {
var dist = Math.ceil((ypos - final_y)/10);
ypos = ypos - dist;
}
elem.style.left = xpos + "px";
elem.style.top = ypos + "px";
var repeat = "moveElement('"+elementID+"',"+final_x+","+final_y+","+interval+")";
elem.movement = setTimeout(repeat,interval);
}
Here are some possible answers I'll give, listed in the order they appeared:
- Use
var moveElement = function (...) syntax instead of function moveElement(...) to make the program more consistent (okay, it's just my personal preference). - Leave spaces after commas in argument list (i.e.
moveElement(elementID, final_x, final_y, interval) instead of moveElement(elementID,final_x,final_y,interval)). Similarly, leave spaces around operators like /. - Either use camel case (better) or underscore case. Don't mix them up (
elementID and final_x). - Always put curly brackets around the body of
if, even when there is only one statement inside (return false in this example). - Always have consistent return type (don't
return false at the beginning, but returning nothing at the end). - Don't declare variables using
var inside if blocks. JavaScript's var keyword only gives function-level locality. - Don't use
var to declare the same variable more than one time (there are var dist for multiple times in the code). - The is no point in assigning zero to
dist at the beginning. - It's a very bad practice to pass a string as the argument to
setTimeout. Pass a function instead. - Using
== to compare is discouraged. Use === instead. - Some code quality tools (JSLint or JSHint) will ask you to supply the second argument (
10) to parseInt. - Don't call to
getElementById each time moveElement is called. Call getElementById just once, and use the element itself in recurring calls. - Similarly, don't call
parseInt again and again. Put numeric values in variables instead.
Here's the revised code I'll have:
var moveElement = function (elementID, finalX, finalY, interval) {
if (!document.getElementById) { return; }
if (!document.getElementById(elementID)) { return; }
var elem = document.getElementById(elementID);
if (elem.movement) {
clearTimeout(elem.movement);
}
var xpos = 0;
var ypos = 0;
var run = function () {
var dist;
if (xpos === finalX && ypos === finalY) {
return;
}
if (xpos < finalX) {
dist = Math.ceil((finalX - xpos) / 10);
xpos = xpos + dist;
}
if (xpos > finalX) {
dist = Math.ceil((xpos - finalX) / 10);
xpos = xpos - dist;
}
if (ypos < finalY) {
dist = Math.ceil((finalY - ypos) / 10);
ypos = ypos + dist;
}
if (ypos > finalY) {
dist = Math.ceil((ypos - finalY) / 10);
ypos = ypos - dist;
}
elem.style.left = xpos + "px";
elem.style.top = ypos + "px";
elem.movement = setTimeout(run, interval);
};
run();
};
What will the frontend developer candidates answer? Maybe they're used to shims, polyfills, transpilers. Will they know the hard years we had with IE 6?