I've been focusing on my PHP skills lately but have been shifting to JavaScript. I'm familiar with the bare-bone basics of jQuery. I'm not as familiar with JavaScript as I'd like to be. I'm a solo-developer so I'd just like somebody to take a look at this and point out any mistakes or things I could be doing better.
What the code does
Creates an arbitrary number of <script> elements, assigns a type and src attribute to each one and then inserts that <script> element before the </body>.
The code
See Better Code below!
function async_init() {
var element, type, src;
var parent = document.getElementsByTagName('body');
var cdn = new Array;
cdn[0] = 'http://code.jquery.com/jquery-1.6.2.js';
cdn[1] = 'https://ajax.googleapis.com/ajax/libs/jqueryui/1.8.14/jquery-ui.js';
for (var i in cdn) {
element = document.createElement('script');
type = document.createAttribute('type');
src = document.createAttribute('src');
type.nodeValue = 'text/javascript';
src.nodeValue = cdn[i];
element.setAttributeNode(type);
element.setAttributeNode(src);
document.body.insertBefore(element, parent);
}
}
<body onload="async_init()">
Right now the code is working, which is great. But my questions:
- Is this the "best" way to do this? I define best as optimum user experience and efficient code.
- Are there any obvious "noobie" flaws in my JavaScript?
- Is there anyway I could get the
onloadattribute out of thebody?
I used Mozilla Developer Network as my JavaScript reference. If there are any other references that are accurate, documented & useful I would love to have more information.
Thanks for checking the code out and your feedback, even if its pointing out how my code sucks!
Better code after critique
function async_init() {
var element;
var cdn = new Array;
cdn[0] = 'http://code.jquery.com/jquery-1.6.2.js';
cdn[1] = 'https://ajax.googleapis.com/ajax/libs/jqueryui/1.8.14/jquery-ui.js';
for (var i in cdn) {
element = document.createElement('script');
element.setAttribute('type', 'text/javascript');
element.setAttribute('src', cdn[i]);
document.body.appendChild(element);
}
}
2 Answers 2
You've erred many times in the above code. However, that means you get to learn a lot about how to properly interact with the DOM. In many instances, there are built-ins that quickly and efficiently get the job done.
First off, you're incorrectly accessing the body. document.body is a well-supported reference to the body that's been around since at least DOM Level 1.
Secondly, you're taking the hard route towards creating an array. In his book, "JavaScript: The Good Parts", Douglas Crockford insists on using literals instead of constructors. In this case, an array literal is []. This is an easier way of calling new Array().
Thirdly, you're incorrectly iterating over the cdn array with a for...in loop instead of a for loop. A for...in loop is meant for objects as it iterates over the properties of an object. Not only is it slower than a normal for loop, but it also is very susceptible to prototype modification. For example, try running this code with a build of MooTools (jsFiddle has one enabled by default):
for(var key in [])
{
console.log(key);
}
You'll get a ton of hits because MooTools modifies the Array prototype. You'd have to do a hasOwnProperty check for each property just to avoid those pitfalls. Avoid for...in on anything but objects.
Next, declaring the i variable inside of the for...in loop gives the illusion of block scope. This isn't the case, since i is available anywhere in the function after it's been defined. Only functions have scope in JavaScript. Here are Crockford's thoughts via "...The Good Parts" (page 102):
In most languages, it is generally best to declare variables at the first site of use. That turns out to be a bad practice in JavaScript because it does not have block scope. It is better to declare all variables at the top of each function.
Finally, setting the attributes for each script tag isn't necessary. Since you're already interacting with the DOM, setting their cousin, the DOM property is a better idea. The DOM provides shortcuts to node properties and is more stable than setting attributes. get/set/removeAttribute all have the unfortunate shortcoming of being very weird in various Internet Explorer builds. They're best avoided unless completely necessary.
Fixed + optimized:
function async_init() {
var element;
var parent = document.body;
var cdn = ["http://code.jquery.com/jquery-1.6.2.js", "https://ajax.googleapis.com/ajax/libs/jqueryui/1.8.14/jquery-ui.js"];
var i = 0, file;
for (i;i<cdn.length;i++) {
file = cdn[i];
element = document.createElement("script");
element.type = "text/javascript";
element.src = file;
parent.appendChild(element);
//free file's value
file = null;
}
/*
* Cleanup at end of function, which isn't necessary unless lots of memory is being
* used up.
* No point nulling a Number, however. In some ECMAScript implementations
* (ActionScript 3), this throws a warning.
*/
//empty array
cdn.splice(0, cdn.length);
//clear variable values
element = null;
parent = null;
cdn = null;
}
Also note that since you're adding these scripts asynchronously, there's no stable way to detect when the file has loaded, short of a script loader or a timer (my opinion is neither are stable).
An alternative is to use document.write to append these scripts synchonously. However, you'll have to use a DOMString and escape the forward slash in the script's end tag. This will append it to the end of the body tag.
Example: document.write("<script type=\"text/javascript\" src=\"\"><\/script>");
Note that the backslash character (\) escapes characters in JavaScript. I've used it to escape the double quotes for each attribute along with the forward slash in the end tag.
If you have any more questions regarding JavaScript or the DOM, feel free to ask in the comments.
-Matt
Looking good.
You may want to look into some of the async loading options of the script tag documentation, as this may be an easier way to accomplish what you are trying to do.
My main thought, though, was you had some extra variables and scoping around building the script element, and that it stands on its own pretty well. To me, this smelled like a separate function:
function async_init() {
addScript('http://code.jquery.com/jquery-1.6.2.js')
addScript('https://ajax.googleapis.com/ajax/libs/jqueryui/1.8.14/jquery-ui.js')
// or your for..in loop
}
function addScript(s) {
const element = document.createElement('script')
element.setAttribute('type', 'text/javascript')
element.setAttribute('src', s)
document.body.appendChild(element)
}
But this definitely a matter of style, and yours is in no way wrong.
setAttributedirectly onelementrather than creating explicit attribute nodes. It'd make the code ever so much less bulky. \$\endgroup\$onload, please. Always useaddEventListener-- though you can only call that once the DOM has been created, so slingshoot it through an initialization function. \$\endgroup\$onloadattribute via the body. It's actually far more stable thanaddEventListener, which falls flat in IE < 8. \$\endgroup\$