patternjavascriptMinor
Page visit counter
Viewed 0 times
counterpagevisit
Problem
The following counts the number of page loads and then does something if <
Can anyone give me any pointers to improve both this function and my JavaScript in general? Both in terms of patterns, design and algorithm.
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
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
variable, instead of calling it repeatedly.
-
In
the
-
You should use
different, and the shorter version should be used sparingly.
-
In
and to pass in a number (
-
In
method. In the first branch of the if, you are using a var in the
global context: Bad Ideatm.
-
In
setting the date. Is this on purpose? Or does
-
In
-
In
-
In
would try to get the index, and then to a single
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 youshould 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 avariable, instead of calling it repeatedly.
-
In
createCookie('counter-cookie',iCount.toString(), '150');, whythe
.toString()?-
You should use
=== and !== instead of == and !=. They aredifferent, 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 themethod. 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 whensetting 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()? Iwould 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.