SitePoint Sponsor

User Tag List

Results 1 to 12 of 12

Thread: JSLint advice

  1. #1
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    48
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)

    JSLint advice

    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)...

    Code JavaScript:
    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?...

    Code JavaScript:
    fields[i].onfocus = (function(item) {
    	if (item.value === item.oldval) {
    		item.value = "";
    	}
    })(this);

    Any help on this would be appreciated.

    Lastly, JSLint also says things like
    Code:
    document
    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!!?

    M.

  2. #2
    om nom nom nom Stomme poes's Avatar
    Join Date
    Aug 2007
    Location
    Netherlands
    Posts
    10,233
    Mentioned
    47 Post(s)
    Tagged
    1 Thread(s)
    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:
    Code:
    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.

  3. #3
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    The functions are created once(well, once per call of fieldReplace()), and the elements are each given a reference to the same function object.
    Code:
    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.

  4. #4
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    48
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Hi, thanks for the replies.

    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"??

    Code JavaScript:
    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;
    				};
    			}
    		})();
    	}
    }

  5. #5
    om nom nom nom Stomme poes's Avatar
    Join Date
    Aug 2007
    Location
    Netherlands
    Posts
    10,233
    Mentioned
    47 Post(s)
    Tagged
    1 Thread(s)
    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...in-javascript/
    I found yesterday seems to describe an example of the variable problem.

  6. #6
    Unobtrusively zen silver trophybronze trophy
    paul_wilkins's Avatar
    Join Date
    Jan 2007
    Location
    Christchurch, New Zealand
    Posts
    14,526
    Mentioned
    83 Post(s)
    Tagged
    3 Thread(s)
    Why can't the closure be moved out to a separate function?
    Programming Group Advisor
    Reference: JavaScript, Quirksmode Validate: HTML Validation, JSLint
    Car is to Carpet as Java is to JavaScript

  7. #7
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    48
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    OK, I changed it around based on James Padosley's article on Closures which was linked to earlier...

    Code JavaScript:
    // 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]);
    	}
    }

    ...seems to not flag up issues in JSLint.

    M.

  8. #8
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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)
    Code:
    // 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
    Code:
     
    // 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;
        }
    }

  9. #9
    om nom nom nom Stomme poes's Avatar
    Join Date
    Aug 2007
    Location
    Netherlands
    Posts
    10,233
    Mentioned
    47 Post(s)
    Tagged
    1 Thread(s)
    Thanks for the code examples, crmalibu. I have a setup somewhat similar to Integralist's and it's nice to learn to rearrange code like you did.

  10. #10
    SitePoint Enthusiast
    Join Date
    Sep 2008
    Posts
    48
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    @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.

    Kind regards,

  11. #11
    SitePoint Wizard bronze trophy
    Join Date
    Jul 2008
    Posts
    5,757
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    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:
    Code:
    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.

  12. #12
    SitePoint Member
    Join Date
    Jan 2010
    Posts
    7
    Mentioned
    0 Post(s)
    Tagged
    0 Thread(s)
    Quote Originally Posted by crmalibu View Post
    There's options in the docs if you want to suppress these notices for certain variables.
    "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."

    like:
    /*global document:false, window: false, self: false */


Bookmarks

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •