Please explain what I'm doing wrong...

S

SirG

I'm looking for an explanation of why one piece of code works and
another does not. I have to warn you that this is the first piece of
Javascript I've ever written, so if there is a better way or a simpler
answer, by all means show me the light!

What I'm trying to do is refresh the page at a timed interval
( actually redirect the page... ) and I have a simple piece of code I
got from the net that works, but I need to modify it a little so I can
call the function and specify the time to wait before redirecting the
page. If that's not clear, hopefully the code will be!

btw, thanks for the help - I really appreciate it!


Here goes.....

----------------- WORKING CODE --------------------
<script language="JavaScript">
<!--
var sTargetURL = "Index.php";
var cacheBuster = parseInt(Math.random()*9999999); // "cache busting"
forces the server to load a 'new' page
function doRedirect2() { setTimeout( "timedRedirect()", 1*700 ); }
function timedRedirect() { window.location.href = sTargetURL + "?cb="
+ cacheBuster; }
//-->
</script>

//Page Call - Working Option 1
<input
type="button"
value="Click me!"
onclick="doRedirect()"
/>
</form>

//Page Call - Working Option 2
<script type="text/javascript">
doRedirect()
</script>

----------------- END of WORKING CODE --------------------


Here's the code that I would LIKE to have working...

----------------- THIS CODE NEEDS HELP --------------------
<script language="JavaScript">
<!--
function redirectPage(interval, timeValueAbbr) {

// check for null value's and set defaults
if (interval == null) { interval = 1; }
if (timeValueAbbr == null) { timeValueAbbr = "ss"; }

// all abbreviations are in MSSQL format: see B.O.L. for datepart()
if (timeValueAbbr == "ss") { timeMultiplier = 1000; }
if (timeValueAbbr == "mi") { timeMultiplier = 1000 * 60; }
if (timeValueAbbr == "hh") { timeMultiplier = 1000 * 60 * 60; }
if (timeValueAbbr == "dd") { timeMultiplier = 1000 * 60 * 60 * 24; }

// create and populate needed variables
var sTargetURL = "Index.php"
var waitTime = interval * timeMultiplier;
var cacheBuster = parseInt( Math.random() * 9999999 ); // "cache
busting" forces the server to load a 'new' page

function doRedirect() { setTimeout( "timedRedirect()", 1*700 ); }
function timedRedirect() { window.location.href = sTargetURL + "?cb="
+ cacheBuster; }

}
//-->
</script>

//Page Call - NOT Working Option 1
<input
type="button"
value="Click me!"
onclick="redirectPage()"
/>
</form>

//Page Call - NOT Working Option 2
<script type="text/javascript">
redirectPage()
</script>

----------------- END of CODE THAT NEEDS HELP --------------------


What am I doing wrong?
 
L

Lee

SirG said:
I'm looking for an explanation of why one piece of code works and
another does not. I have to warn you that this is the first piece of
Javascript I've ever written, so if there is a better way or a simpler
answer, by all means show me the light!

What I'm trying to do is refresh the page at a timed interval
( actually redirect the page... ) and I have a simple piece of code I
got from the net that works, but I need to modify it a little so I can
call the function and specify the time to wait before redirecting the
page. If that's not clear, hopefully the code will be!

btw, thanks for the help - I really appreciate it!
Click to expand...
What am I doing wrong?
Click to expand...

If all you want to do is to automatically redirect this
page after a certain number of seconds, just add one
line to the <head> section of your page:

<meta http-equiv="refresh" content="5; url=Index.php">

Replace the 5 with your time interval in seconds.


But, if you want to see the Javascript solution fixed
so that it works:

Your first mistake was to copy code from a bad source.
You should never apply parseInt() to the result of a
mathematical operation, and there's no need for the
"cacheBuster" value to be an integer, anyway.

Then you did a lot of extra work to convert a time
interval and units value to a "waitTime", but never
passed in the interval or units or used the waitTime
variable.

Then you had a couple of nested function definitions
that, besides being illegal, weren't going to do
anything.

The following code redirects to the URL named in
sTargetURL after the number of seconds specified
in the onClick handler of the button.

<html>
<head>
<script type="text/javascript">
function redirectPage(sec) {
// create and populate needed variables
var sTargetURL = "Index.php"
var waitTime = sec * 1000;
var cacheBuster = Math.random();
// "cache busting" forces the server to load a 'new' page
setTimeout( "location.href='"+sTargetURL+"?cb="+cacheBuster+"'", waitTime );
}
</script>
</head>
<body>
<input
type="button"
value="Click me!"
onclick="redirectPage(5)"</form>
</body>
</html>


--
 
P

Peter Michaux

I'm looking for an explanation of why one piece of code works and
another does not. I have to warn you that this is the first piece of
Javascript I've ever written, so if there is a better way or a simpler
answer, by all means show me the light!
Click to expand...

This might be helpful...

<script language="JavaScript">
Click to expand...

The language attribute is deprecated. Use type="text/javascript"
instead
<!--
Click to expand...

The code commenting trick stopped being useful in 1997 or something
similar
function redirectPage(interval, timeValueAbbr) {

// check for null value's and set defaults
if (interval == null) { interval = 1; }
Click to expand...

You can write
if (!interval) {interval = 1;}

or even

interval = interval || 1;
if (timeValueAbbr == null) { timeValueAbbr = "ss"; }
Click to expand...

No need for the above line if you use a default case in your if's
below
// all abbreviations are in MSSQL format: see B.O.L. for datepart()
if (timeValueAbbr == "ss") { timeMultiplier = 1000; }
Click to expand...


You haven't decalred timeMultiplier and so it is an implied global
variable. Somewhere inside this function you need to say

var timeMultiplier;
if (timeValueAbbr == "mi") { timeMultiplier = 1000 * 60; }
if (timeValueAbbr == "hh") { timeMultiplier = 1000 * 60 * 60; }
if (timeValueAbbr == "dd") { timeMultiplier = 1000 * 60 * 60 * 24; }
Click to expand...

no need to check these conditionals if a previous one was correct

// create and populate needed variables
var sTargetURL = "Index.php"
var waitTime = interval * timeMultiplier;
var cacheBuster = parseInt( Math.random() * 9999999 ); // "cache busting" forces the server to load a 'new' page
Click to expand...

No need for parseInt above. (When using parseInt always use the second
radix parameter.)

This forces the *browser* to load a new page

There is a chance the random number will repeat and not work as a
"cache buster". Use Date instead

var cacheBuster = (new Date()).getTime();

function doRedirect() { setTimeout( "timedRedirect()", 1*700 ); }
Click to expand...

I think this is the problem for your non-working code.

setTimeout can take a string or function as the first argument. The
function choice seems to be preferred for performance reasons but that
doesn't really affect you here.

When you use a string as the argument to setTimeout, the string is
evaluated in the scope of the global window object. Since you have
defined timedRedirect inside your redirectPage function, when the
string is evaluated it cannot find the timedRedirect function.

You probably meant to involve waitTime in the second argument to
setTimeout

also you never call the doRedirect function so the setTimeout is never
called either and the redirect will never happen.
function timedRedirect() { window.location.href = sTargetURL + "?cb=" + cacheBuster; }
}

//-->
Click to expand...

Don't need the above comment line
</script>
Click to expand...

To combine all this try

<script type="text/javascript">

function redirectPage(interval, timeValueAbbr) {
// check for null value's and set defaults
interval = interval || 1;

var timeMultiplier;
// all abbreviations are in MSSQL format: see B.O.L. for
datepart()
if (timeValueAbbr == "mi") {
timeMultiplier = 1000 * 60;
} else if (timeValueAbbr == "hh") {
timeMultiplier = 1000 * 60 * 60;
} else if (timeValueAbbr == "dd") {
timeMultiplier = 1000 * 60 * 60 * 24;
} else {
timeMultiplier = 1000;
}

// create and populate needed variables
var waitTime = interval * timeMultiplier;
var sTargetURL = "Index.php" + (new Date()).getTime(); // "cache
busting" forces the browser to load a 'new' page

setTimeout(function() {window.location.href = sTargetURL;},
waitTime);
}

</script>

Your life will be easier with <URL: http://getfirebug.com> installed.

Peter
 
T

Tim Streater

I'm looking for an explanation of why one piece of code works and
another does not. I have to warn you that this is the first piece of
Javascript I've ever written, so if there is a better way or a simpler
answer, by all means show me the light!
Click to expand...

This might be helpful...
[snip]
// check for null value's and set defaults
if (interval == null) { interval = 1; }
Click to expand...

You can write
if (!interval) {interval = 1;}[/QUOTE]

On the other hand what the OP wrote is much clearer.
or even

interval = interval || 1;
Click to expand...

I'm not sure if this is even correct. What about if interval was 10?
 
S

SirG

Thank you very much for your help!

Here's a point by point reply:

1. The metatag is a helpful option but not quite what I needed here,
but thanks for the suggestion.
2. Copying code from a bad source: that's what I get for not knowing
javascript or the validity/competency of the coders I was 'learning'
from. Are there a couple sites that you would recommend I visit for
javascript tutorials ( other than w3schools )
3. Unused variable ( waitTime ): I had originally used that in place
of the 1*700 in the setTimeout function but because I was having
problems I copied and pasted the values from the working code and had
not replaced that with the waitTime variable.
4. Nested Functions: Thanks for the tip, I'll be sure and stay away
from that in the future. But that begs the question of why it worked
in the original code - any thoughts?

Thanks again for helping me out and explaining what I did wrong!

Steve
 
S

SirG

This might be helpful...



The language attribute is deprecated. Use type="text/javascript"
instead


The code commenting trick stopped being useful in 1997 or something
similar



You can write
if (!interval) {interval = 1;}

or even

interval = interval || 1;


No need for the above line if you use a default case in your if's
below


You haven't decalred timeMultiplier and so it is an implied global
variable. Somewhere inside this function you need to say

var timeMultiplier;


no need to check these conditionals if a previous one was correct


No need for parseInt above. (When using parseInt always use the second
radix parameter.)

This forces the *browser* to load a new page

There is a chance the random number will repeat and not work as a
"cache buster". Use Date instead

var cacheBuster = (new Date()).getTime();


I think this is the problem for your non-working code.

setTimeout can take a string or function as the first argument. The
function choice seems to be preferred for performance reasons but that
doesn't really affect you here.

When you use a string as the argument to setTimeout, the string is
evaluated in the scope of the global window object. Since you have
defined timedRedirect inside your redirectPage function, when the
string is evaluated it cannot find the timedRedirect function.

You probably meant to involve waitTime in the second argument to
setTimeout

also you never call the doRedirect function so the setTimeout is never
called either and the redirect will never happen.



Don't need the above comment line


To combine all this try

<script type="text/javascript">

function redirectPage(interval, timeValueAbbr) {
// check for null value's and set defaults
interval = interval || 1;

var timeMultiplier;
// all abbreviations are in MSSQL format: see B.O.L. for
datepart()
if (timeValueAbbr == "mi") {
timeMultiplier = 1000 * 60;
} else if (timeValueAbbr == "hh") {
timeMultiplier = 1000 * 60 * 60;
} else if (timeValueAbbr == "dd") {
timeMultiplier = 1000 * 60 * 60 * 24;
} else {
timeMultiplier = 1000;
}

// create and populate needed variables
var waitTime = interval * timeMultiplier;
var sTargetURL = "Index.php" + (new Date()).getTime(); // "cache
busting" forces the browser to load a 'new' page

setTimeout(function() {window.location.href = sTargetURL;},
waitTime);
}

</script>

Your life will be easier with <URL:http://getfirebug.com> installed.

Peter
Click to expand...

Wow! There's sooo much I was doing wrong - thanks for pointing out all
the obsolete things in my code, I'll have those switched in no time.
I'm glad you caught the optimization possibility with the multiple if
statements. There's certainly no use in checking all the options if
it's not necessary!

I'll change the Math() to Date() although for what I'm doing I don't
think it's entirely needed, but there's no harm in doing it the
'right' way just in case it becomes an issue.

Thanks a ton!
 
S

SirG

I'm not sure if this is even correct. What about if interval was 10?

I'm not sure what you mean by this. Could you clarify your question?
 
S

SirG

Thank you all very much for your help, I really appreciate it! Here's
my working version of the script using some of the advice you've given
me.

<script type="text/javascript">
function redirectPage(interval, timeValueAbbr) {

// check for null value's and set defaults
if ( !interval ) { interval = 1; } // I decided not to use the
interval = interval || 1 because it's a bit confusing to me... but
maybe I'll get used to it later *shurg*
if ( !timeValueAbbr ) { timeValueAbbr = "ss"; }

// create and populate needed variables
var waitTime = null;
var sTargetURL = "Index.php" + "?cb=" + (new Date()).getTime(); //
"cache busting" forces the browser to load a 'new' page

// all abbreviations are in MSSQL format: see B.O.L. for datepart()
if (timeValueAbbr == "ss") { waitTime = interval * 1000; }
else if (timeValueAbbr == "mi") { waitTime = interval * 1000 * 60; }
else if (timeValueAbbr == "hh") { waitTime = interval * 1000 * 60 *
60; }
else if (timeValueAbbr == "dd") { waitTime = interval * 1000 * 60 *
60 * 24; }
else
{
alert("The time value you've selected is invalid");
return false;
}

setTimeout( function() {window.location.href = sTargetURL;} ,
waitTime );
}
</script>

<script type="text/javascript">
redirectPage(2, 'ss')
</script>
 
P

Peter Michaux

<script type="text/javascript">
function redirectPage(interval, timeValueAbbr) {

// check for null value's and set defaults
if ( !interval ) { interval = 1; } // I decided not to use the
interval = interval || 1 because it's a bit confusing to me... but
maybe I'll get used to it later *shurg*
Click to expand...

Not much difference. One save characters which matters a bit with
JavaScript
if ( !timeValueAbbr ) { timeValueAbbr = "ss"; }

// create and populate needed variables
var waitTime = null;
Click to expand...

You don't need to say "= null" for any particular reason here.
var sTargetURL = "Index.php" + "?cb=" + (new Date()).getTime(); //
"cache busting" forces the browser to load a 'new' page

// all abbreviations are in MSSQL format: see B.O.L. for datepart()
if (timeValueAbbr == "ss") { waitTime = interval * 1000; }
else if (timeValueAbbr == "mi") { waitTime = interval * 1000 * 60; }
else if (timeValueAbbr == "hh") { waitTime = interval * 1000 * 60 *
60; }
else if (timeValueAbbr == "dd") { waitTime = interval * 1000 * 60 *
60 * 24; }
else
{
alert("The time value you've selected is invalid");
return false;
}

setTimeout( function() {window.location.href = sTargetURL;} ,
waitTime );}

</script>

<script type="text/javascript">
redirectPage(2, 'ss')
</script>
Click to expand...

Looks like one good way to do it.

Did you install Firebug and order Flangan's book? :)

Peter
 
D

Dr J R Stockton

In comp.lang.javascript message <[email protected]
glegroups.com>, Thu, 5 Jul 2007 14:39:54, SirG
I have to warn you that this is the first piece of
Javascript I've ever written, so if there is a better way or a simpler
answer, by all means show me the light!
Click to expand...

Having read 7 articles in the thread : in response to the Subject line -
clearly you should have read the newsgroup FAQ. First.

var cacheBuster = parseInt(Math.random()*9999999); // "cache busting"
Click to expand...

That multiplies two Numbers, giving a Number; parseInt needs a String,
so there's a costly conversion, and parseInt gives a Number which will
be converted to String later.
Use var cacheBuster = (Math.random()*9999999)|0
or var cacheBuster = Math.floor(Math.random()*9999999)
but var cacheBuster = + new Date()
though perhaps slower is IMHO more elegant.
forces the server to load a 'new' page
function doRedirect2() { setTimeout( "timedRedirect()", 1*700 ); }
Click to expand...

But 1*700 is 700 - no need for the 1* .

What am I doing wrong?
Click to expand...

Copying bad code.


It's a good idea to read the newsgroup c.l.j and its FAQ. See below.
 
S

SirG

@Peter:

Actually, I've been using firebug for a while (.8 or something) but
I've used it mainly for digging into the css of various sites to see
how they're done. I've looked at the javascript console but ran away
in terror :0) I'll have to take another look at it now. I've seen the
Yahoo! video of Joe Hewitt explaining how it works but it is a bit
over my head at the moment. *shrug*

I have not yet ordered Flangan's book, but thanks to -Lost- I'll end
up ordering the 5th edition when I do. ( I just ordered a new 580 EX
II flash + lightsphere diffuser for my camera, so I'm not sure my wife
will care much for a new book but maybe now is the time! ) ;0)

Couple quick questions from your last post.

1. The extra characters in my if() statements. Is the extra character
count a problem because js should be lean and mean or is there a
performance hit for longer style syntax?
2. You said the "= null" is not necessary... is that because the
variable are null by default?


Again, thanks a bunch for all the help! ( for everyone involved, but
especially Peter! )
 
P

Peter Michaux

Oh! On that note, make sure to get the 5th edition. I have it, and
whilst there are still some major no-nos (by this group's standards), it
is exponentially better. I upgraded from the 3rd.
Click to expand...

I still find the 4th edition quite handy. The index in the 5th edition
is my biggest complaint. There are many times where the page I'm
looking for is not included in the appropriate entry in the index.

Everyone should watch out for errata in Flanagan's book. It is the
best JavaScript book I've read but there are quite a few mistakes and
many are listed on the O'Reilly site.
In this edition he covers Ajax, DOM scripting, and finally, something
I've never before seen in any JavaScript book or manual, the coverage of
client-side graphics scripting. That's right SVG fans, Mr. Flanagan
aims to please!

Also, something that I have been experimenting with for some time,
client-side persistence. He hasn't quite covered the uber-elite "DOM
storage" methods, but he definitely gives you some ideas inside
per-session persistence.

http://www.davidflanagan.com/blog/2006_08.html#000110
Click to expand...

I really don't get all the current the fuss about client-site
persistence. If the user changes browsers or computers then it is
worthless. If the web is a client-server model then the data should be
on the server. Also the world is getting more connected rather than
more disconnected so intermittent connections are less likely. Please
enlighten me.

Peter
 
P

Peter Michaux

@Peter:

Actually, I've been using firebug for a while (.8 or something) but
I've used it mainly for digging into the css of various sites to see
how they're done. I've looked at the javascript console but ran away
in terror :0) I'll have to take another look at it now. I've seen the
Yahoo! video of Joe Hewitt explaining how it works but it is a bit
over my head at the moment. *shrug*
Click to expand...

Upgrading to the newest Firebug 1.* will be a performance boost and
many more features.

The CSS stuff is amazing. Seeing which rules are overridden is such a
time saver.

I don't use the debugger. That scares me. Just the regular console is
fine for me. Try doing this in some script in a page

console.log('my message');
console.log(window);
var obj = {a:1,b:2};
console.log(obj);

Just that level of use alone makes my life better. It is like using
alert() but without having to click ok all the time.

Couple quick questions from your last post.

1. The extra characters in my if() statements. Is the extra character
count a problem because js should be lean and mean or is there a
performance hit for longer style syntax?
Click to expand...

Since JavaScript is downloaded the more characters the longer the
download. It is something to keep in mind at times but not at the
expense of maintainability or readability, in my opinion. However I do
think people are wasteful frequently.
2. You said the "= null" is not necessary... is that because the
variable are null by default?
Click to expand...

by default they are "undefined". Try this in the firebug console :)

var x;
x === undefined

and you should see true. Then do all these and get confused

0 == ''
0 == false
0 == undefined
0 == null
null == ''
null == false
null == undefined
etc

This is why Douglas Crockford recommends always using === when
comparing potentially "falsey values". I think === is what they really
meant to design when they designed ==.
Again, thanks a bunch for all the help! ( for everyone involved, but
especially Peter! )
Click to expand...

Glad to help (and procrastinate on my project.)

Peter
 
S

SirG

Glad to help (and procrastinate on my project.)
Click to expand...

Ohh, I know how that goes - glad to hear I'm not the only one that
does it!

I've got one last question for you.

Say I want to modify the script so it accepts three values, like so:

function redirectPage(interval, timeValueAbbr, file) {
....
}

What code do I need so the default 'file' is set to the webpage that
called the js?
 
P

Peter Michaux

Ohh, I know how that goes - glad to hear I'm not the only one that
does it!

I've got one last question for you.

Say I want to modify the script so it accepts three values, like so:

function redirectPage(interval, timeValueAbbr, file) {
...

}

What code do I need so the default 'file' is set to the webpage that
called the js?
Click to expand...

You can include the URL of the current page in the URL similarly to
how you use the cache buster string as part of the query string

var sTargetURL = "Index.php?cb=" + (new Date()).getTime() +
"&currentPage=" +
encodeURIComponent(window.location.href);

The encodeURIComponent() function escapes characters that can't be the
value of a query string parameter.

---

By the way, the cache buster you are using is not guaranteed to work.
Browsers are not required to look a the URL query string when
determining whether or not to pull from the cache.

Peter
 
S

SirG

You can include the URL of the current page in the URL similarly to
how you use the cache buster string as part of the query string

var sTargetURL = "Index.php?cb=" + (new Date()).getTime() +
"&currentPage=" +
encodeURIComponent(window.location.href);
Click to expand...

Excellent, thank you!


By the way, the cache buster you are using is not guaranteed to work.
Browsers are not required to look a the URL query string when
determining whether or not to pull from the cache.
Click to expand...

What is the more correct way of doing this?
 
S

SirG

Having read 7 articles in the thread : in response to the Subject line -
clearly you should have read the newsgroup FAQ. First.
Click to expand...

Yes, yes I should have... Now I'll know for next time.

That multiplies two Numbers, giving a Number; parseInt needs a String,
so there's a costly conversion, and parseInt gives a Number which will
be converted to String later.
Click to expand...

Several others mentioned that as well, so I've removed the parseInt()
code. Honestly, I didn't think it was nessisary but not knowing what
was going on behind the scenes I left it in until such a time as I
knew one way or another if it was really needed.
But 1*700 is 700 - no need for the 1* .
Click to expand...

This was originally changed to 1, 2, 3 ... for each page. Like I said,
this is all new to me but, but I think the 1*700 issue is nicely
resolved. :0)

Copying bad code.
Agreed.

It's a good idea to read the newsgroup c.l.j and its FAQ. See below.

--
(c) John Stockton, Surrey, UK. [email protected] Turnpike v6.05 IE 6
FAQ <URL:http://www.jibbering.com/faq/index.html>.
<URL:http://www.merlyn.demon.co.uk/js-index.htm> jscr maths, dates, sources.
<URL:http://www.merlyn.demon.co.uk/> TP/BP/Delphi/jscr/&c, FAQ items, links.
Click to expand...

I have actually taken a look through the clj faq and interestingly
enough the reccomendation for cache busting there is to use
location.replace + new Date().valueOf(). Hmmph, actually @peter, I
think I just answered my own question to you... *sigh*

Ohh, and Sir Stockton - that's a pretty massive website you have
there! Thanks for the link.
 
S

SirG

By the way, the cache buster you are using is not guaranteed to work.
Browsers are not required to look a the URL query string when
determining whether or not to pull from the cache.
Click to expand...

Please disregard my earlier question to you...

I actually read (instead of skimming) the "how to I force a reload
from the server/prevent caching?" question in the clj faq (4.17), so
I've answered my own question!

Amazing how that works... actually read something instead of skimming
it and *BAM* you learn something... who would have thought?!

:0)
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Similar Threads

What "might" I be doing wrong with this snippet? 6
Please Help? 0
location.href--what am i doing wrong? 4
Help With a Script 5
Survey details won't go through using php, ajax, Mysql 0
any javascript expert out there? help please! 2
Events not firing (Clearly I'm doing something wrong) 4
What Am I doing wrong here? 4

Members online

Total: 29 (members: 2, guests: 27)
Robots: 312

Forum statistics

Threads
473,755
Messages
2,569,536
Members
45,007
Latest member
obedient dusk

Latest Threads

Top