Can anyone shed any light on some of the issues that JSLint has raised with my code. I seem to have the same issues every time I run my code through JSLint and I’m not sure how to resolve them.
For example, JSLint says “Be careful when making functions within a loop. Consider putting the function in a closure.”.
But how would I (for instance) go about converting the following code into a Closure (JSLint says the “fields[i].onfocus” and “fields[i].onblur” event handlers are the problem)…
function fieldReplace(o) {
var fields = o.getElementsByTagName("input");
for (var i=0, len=fields.length; i<len; i++) {
// Set custom attribute to retrieve original value if necessary
fields[i].oldval = fields[i].value;
// Now reset current value of the field when in focus
fields[i].onfocus = function() {
// If the current value hasn't yet been changed then set the value to blank so the user can enter a new value
if (this.value === this.oldval) {
this.value = "";
}
};
// Now restore the original value of the field when blur
fields[i].onblur = function() {
// If the value isn't blank then it means the user has entered a new value and so there is no need to restore the original value
if (this.value === "") {
this.value = this.oldval;
}
};
}
};
…I did try changing that part of the code to what I believed would work but JSLint still displayed the same error message?..
are “implied global”. How do you fix this apparent issue? I tried being more specific and changing references of “document” to “window.document” but that just added another instance of “window” to the implied global list!!?
You don’t want to add “window” anyway as it’s more overhead. Technically you can put window in front of anything, but as an implied global object it just makes JS check who “window” is, realise it points to “the global object” and then cycles back to window again.
I had a similar problem and someone said “check the Expect In A Browser setting” though nothing changed when I did that, but it’s supposed to know that window and document ARE global objects in a browser.
For example, JSLint says “Be careful when making functions within a loop. Consider putting the function in a closure.”
That might be performance, since the function gets initialised every time the loop goes through its loopiness? I’d like to know the reason too.
I would like to see how to use a closure with this too. I read that closures are good for when you want some function to be able to grab a variable or value from another function, which means the for loop would be inside some wrapping function and that wrapping function would define the onfocus function, and after that wrapper function is done (exits), the onfocus function is supposed to be able to grab those fields[i] values and work with them. But since they’re changing as the user is doing their onfocussing and onblurring, I’d also like to see how this would work.
*edit
Oh lemme try, I have to learn how to do this too:
function fieldReplace(o) {
var fields = o.getElementsByTagName("input");
var len = fields.length; //define it outside if there are tons of inputs
for (var i=0; i<len; i++) {
fields[i].oldval = fields[i].value;
fields[i].onfocus = function() {
return function() {
if (this.value === this.oldval) {
this.value = "";
}
};
}(i);
fields[i].onblur = function() {
return function() {
if (this.value === "") {
this.value = this.oldval;
}
};
}(i);
}
};
Hm, I’m not sure how that goes with two of them returning nameless functions, but it’s something sorta like that.
Hoping Raffles or someone comes along with a nice example of how to do it right.
The functions are created once(well, once per call of fieldReplace()), and the elements are each given a reference to the same function object.
function fieldReplace(o) {
var fields = o.getElementsByTagName("input");
function myOnFocus() {
// If the current value hasn't yet been changed then set the value to blank so the user can enter a new value
if (this.value === this.oldval) {
this.value = "";
}
}
function myOnBlur() {
// If the value isn't blank then it means the user has entered a new value and so there is no need to restore the original value
if (this.value === "") {
this.value = this.oldval;
}
}
for (var i=0, len=fields.length; i<len; i++) {
// Set custom attribute to retrieve original value if necessary
fields[i].oldval = fields[i].value;
// Now reset current value of the field when in focus
fields[i].onfocus = myOnFocus;
// Now restore the original value of the field when blur
fields[i].onblur = myOnBlur;
}
}
I wouldn’t worry about implied global for stuff like document, window etc…It’s a notice meant to draw your attention in case you forgot to declare a certain variable. There’s options in the docs if you want to suppress these notices for certain variables. jslint isn’t just for javascript in the browser enviornment, it’s more general than that.
I looked up closures in John Resig’s book “Pro JavaScript Techniques” and followed his example (see below code which shows a working closure) but JSLint STILL! shows the warning of “Be careful when making functions within a loop. Consider putting the function in a closure”??
function External() {
// Private method for opening a popup window
var popup = function()
{
var url = arguments[0];
var w = arguments[1];
var h = arguments[2];
var toggle = arguments[3];
// Set x and y positions of new window
x = screen.width / 2 - ( w / 2 );
y = screen.height / 2 - ( h / 2 );
// Open window to specific
window.open( url, 'popup', 'location=yes, resizable=yes, width=' + w + ',height=' + h + ',scrollbars=' + toggle + ',left=' + x + ',top=' + y );
};
// first store all anchor elements in an array
var a = document.getElementsByTagName('a');
// Store array length in variable
var len = a.length;
// Loop through the array checking for any A elements with an "rel" attribute that equals "external"
for (var i=0; i<len; i++) {
(function(){
// Remember the element within this scope
var element = a[i];
// Now we can properly refer to the element within the closure
if (a[i].getAttribute( 'rel' ) == 'external') {
element.onclick = function() {
popup( element.href, screen.availWidth, screen.availHeight, 1 ); return false;
};
}
})();
}
}
It probably still warns because even though they’re in a closure, the closure itself is still inside the for loop (so Lint is still going to say, “you have a function in a loop”). But John’s version might fix the main problem with functions in loops, which seems to be grabbing the correct element when looping through elements: http://james.padolsey.com/javascript/closures-in-javascript/
I found yesterday seems to describe an example of the variable problem.
OK, I changed it around based on James Padosley’s article on Closures which was linked to earlier…
// Private scope method for opening a popup window
var popup = function(node) {
var url = node.href,
settings = 'location=yes,resizable=yes,width=' + screen.availWidth + ',height=' + screen.availHeight + ',scrollbars=1,left=0,top=0';
return function() {
window.open(url, 'external' , settings);
return false;
};
};
// Private variable to store HTMLCollection of all <A> elements
var a = document.getElementsByTagName('a');
// Private variable to store HTMLCollection length
var len = a.length;
// Loop through the array checking for any A elements with an "rel" attribute that equals "external"
for (var i=0; i<len; i++) {
if (a[i].getAttribute( 'rel' ) == 'external') {
a[i].onclick = popup(a[i]);
}
}
All you’ve really done is outsmart jslint. That shouldn’t be your goal. You’re still “making a function in a loop”.
It’s just a notice to draw your attention to something that might stand to be improved upon. Sometimes you need to make a function in a loop, because it needs to be a closure, or just plain needs to be a distinct object for other purposes. If it doesn’t, then it’s unnecessary, and that’s what the notice is for.
This does essentially the same(in this example)
// Private variable to store HTMLCollection of all <A> elements
var a = document.getElementsByTagName('a');
// Private variable to store HTMLCollection length
var len = a.length;
var settings = 'location=yes,resizable=yes,width=' + screen.availWidth + ',height=' + screen.availHeight + ',scrollbars=1,left=0,top=0';
// Loop through the array checking for any A elements with an "rel" attribute that equals "external"
for (var i=0; i<len; i++) {
if (a[i].getAttribute( 'rel' ) == 'external') {
a[i].onclick = function(){
window.open(this.href, 'external' , settings);
return false;
};
}
}
This works too, but without “making a function in a loop”. Of course, we got lucky because you can use the implied object to get a reference to the dom node in this situation
// Private variable to store HTMLCollection of all <A> elements
var a = document.getElementsByTagName('a');
// Private variable to store HTMLCollection length
var len = a.length;
var settings = 'location=yes,resizable=yes,width=' + screen.availWidth + ',height=' + screen.availHeight + ',scrollbars=1,left=0,top=0';
function popup() {
window.open(this.href, 'external' , settings);
return false;
}
// Loop through the array checking for any A elements with an "rel" attribute that equals "external"
for (var i=0; i<len; i++) {
if (a[i].getAttribute( 'rel' ) == 'external') {
a[i].onclick = popup;
}
}
@crmalibu thanks for the reply but your point has confused me a little as your second example shows you avoiding exactly what JSLint is advising you should avoid (e.g. “Be careful when making functions within a loop”) and is very similar to what I have done in my last code example, i.e. moving the function outside of the loop! But you say that my last code example is simply “outsmarting” JSLint? Surely what I’ve done is what JSLint has asked for which is to be careful using a function from within a loop?
You then followed with
You’re still “making a function in a loop”.
but if you look at my last code example I’ve now moved the function OUTSIDE the loop?
Or are you referring to the fact that I’m executing the function “popup” from within the assignment?
Sorry if I missed your point or I’m misunderstanding the problem, maybe you can clarify for me.
What you did was create a function, that, every time it’s called, creates and returns a new function. I was showing you an equivalent peice of code that jslint would warn you about because it can detect the function creation inside of the loop.
Thinking about it more, I think maybe I’ve misunderstood the reason jslint warns about this. It might be complaining because of stuff like this:
var funcs = [];
var funcs = [];
for (var i=0; i<3; i++) {
funcs.push(function(){
alert("i am func #" + i);
});
}
//all of them claim to be func #3
funcs[0]();
funcs[1]();
funcs[2]();
jslint is probably worried about the possibly unintentional closure created around the i variable. Or in your case, it would have been the element variable.
The way you went about solving it(calling a function that returns another function) is a good way to do it.
“JSLint also recognizes a /*global */ comment that can indicate to JSLint that variables used in this file were defined in other files. The comment can contain a comma separated list of names. Each name can optionally be followed by a colon and either true or false, true indicated that the variable may be assigned to by this file, and false indicating that assignment is not allowed which is the default.”