Page visit counter

Question

The following counts the number of page loads and then does something if < MIN_PAGE_VIEWS.

Can anyone give me any pointers to improve both this function and my JavaScript in general? Both in terms of patterns, design and algorithm.

// Call on body onload event.
// Checks to see if prompt is to be displayed and if banner is to be displayed.
function bodyOnLoad() { 

    var MIN_PAGE_VIEWS = 3;

    if(readCookie('counter-cookie') == null){   
        createCookie('counter-cookie', '0', '150');
    }

    // Only start showing random popup after second page visit
    if (readCookie('counter-cookie') < MIN_PAGE_VIEWS) { 

        var iCount = parseInt(readCookie('counter-cookie'));
        iCount++;
        createCookie('counter-cookie',iCount.toString(), '150');
    } 
    else{
        if (readCookie('ab5cd57e-871a-4b65') != 'yes') { 
            showPanel('popupPanel',80,240,'yes'); 
        }
    } 

    if(readCookie('banner-cookie') == 'yes'){
        showPanel('surveyBanner',100,100, 'no'); 
        displayBanner();
    }
}

// New Cookie and Set expiration date
function createCookie(name,value,days) {
    if (days) {
        var date = new Date();
        date.setTime(date.getTime()+(days*24*60*60*1000));
        var expires = "; expires="+date.toGMTString();
    }
    else var expires = "";
    document.cookie = name+"="+value+expires+"; path=/";
}

// Separates cookies
function readCookie(name) {
    var nameEQ = name + "=";
    var ca = document.cookie.split(';');
    for(var i=0;i < ca.length;i++) {
        var c = ca[i];
        while (c.charAt(0)==' ') c = c.substring(1,c.length);
        if (c.indexOf(nameEQ) == 0) return c.substring(nameEQ.length,c.length);
    }

    return null;
}

Answer 1

If you want to improve your JavaScript, run your code through http://www.jslint.com/
Where it complaints about something, try to understand why - more often than not, it is a valid complaint. :)

---

1. Start the JavaScript with "use strict";. (That also means you should wrap the code in a self-executing function.) There is plenty of material out there explaining this much better than I would. :)

2. You should store the result of readCookie('counter-cookie') in a variable, instead of calling it repeatedly.

3. In createCookie('counter-cookie',iCount.toString(), '150');, why the .toString()?

4. You should use === and !== instead of == and !=. They are different, and the shorter version should be used sparingly.

5. In createCookie, you want if(typeof days === 'number') { ... }, and to pass in a number (150) instead of a string ('150').

6. In createCookie, declare the var expires at the top of the method. In the first branch of the if, you are using a var in the global context: Bad Idea^tm.

7. In createCookie, you seem to be overriding all cookies when setting the date. Is this on purpose? Or does document-cookie = 'foo'; do some black-magic that I am not expecting?

8. In createCookie you use date.toGMTString() - in MDN it is advised to use .toUTCString instead.

9. In readCookie, that indention is hurtful. :|

10. In readCookie, do you need to while(!false) str.substring()? I would try to get the index, and then to a single substring() op.

Answer 2

• Always declare variables on the first line of their function. They are automatically hoisted there anyway, so declaring them anywhere else results in code that is written in a different order than it is executed.
• Keep your formatting consistent. jslint has a particular style that is considered "proper", but obeying their rules exactly is less important than never breaking your own rules.
• Namespace your code. An easy way to do this is to wrap your code in (function () {/* code here */}());. The reason you want to do this is so you don't accidentally overwrite other scripts' variables, and so they don't overwrite yours.
• Try to keep results around that you might need again. For example, readCookie() currently re-parses document.cookie every time it is called, when it really only needs to parse it once, then keep the result around. Of course, then the array isn't updated by createCookie, but it doesn't look like your code requires that functionality.
• Be aware of the difference between === and == - in several cases above, you used the latter when you wanted the former.
• Storing the string "yes" to indicate true overcomplicates things. I would just set the cookie to 1 (which is automatically converted to "1"), then, when testing for it, simply write if (readCookie('banner-cookie')) {.
• Some would disagree with me on this one, but I think regular expressions can make your code more readable. They can certainly make it shorter.