HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Page visit counter

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
counterpagevisit

Problem

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;
}

Solution

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

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

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

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

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

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

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

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

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

-
In readCookie, that indention is hurtful. :|

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

Context

StackExchange Code Review Q#11939, answer score: 3

Revisions (0)

No revisions yet.