Please explain what I'm doing wrong...

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

[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!

>What am I doing wrong?

If all you want to do is to automatically redirect this
page after a certain number of seconds, just add one
line to the <headsection 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>
--

[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!

This might be helpful...

<URL: http://groups.google.com/group/comp.lang.javascript/msg/be461c04829081ce>

<script language="JavaScript">

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

<!--

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

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

or even

interval = interval || 1;

if (timeValueAbbr == null) { timeValueAbbr = "ss"; }

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

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

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

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

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

//-->

Don't need the above comment line

</script>

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

Peter

[Tim Streater]

In article <11**********************@i13g2000prf.googlegroups .com>,
Peter Michaux <pe**********@gmail.comwrote:

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!

This might be helpful...

[snip]

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

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

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

or even

interval = interval || 1;

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

[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

[SirG]

On Jul 5, 11:43 am, Peter Michaux <[e-mail]:

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!

This might be helpful...

<URL:http://groups.google.com/group/comp.lang.javascript/msg/be461c04829081ce>

<script language="JavaScript">

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

<!--

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

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

or even

interval = interval || 1;

if (timeValueAbbr == null) { timeValueAbbr = "ss"; }

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

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

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

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

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

//-->

Don't need the above comment line

</script>

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

Peter

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!

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

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

[-Lost]

Tim Streater wrote:

In article <11**********************@i13g2000prf.googlegroups .com>,
Peter Michaux <pe**********@gmail.comwrote:

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

This might be helpful...

[snip]

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

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

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

No, it isn't actually. The former is also prone to more error. Take
into consideration interval being set to 0. The negation operator
(logical NOT) takes this into account, testing against null does not.

>or even

interval = interval || 1;

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

It is. And you'd do wise to not run around trying to correct
established members of this group. Or at least the ones who know their
shite.

Also, if interval is 10, then interval will be set to 10. If interval
is not set, or does not resolve to a true statement (such as 10, true,
whatever) it will be set to 1 as a default.

--
-Lost
Remove the extra words to reply by e-mail. Don't e-mail me. I am
kidding. No I am not.

[Peter Michaux]

On Jul 5, 1:08 pm, SirG <[e-mail]:

@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*

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?

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?

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

Glad to help (and procrastinate on my project.)

Peter

[SirG]

Glad to help (and procrastinate on my project.)

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?

[Peter Michaux]

On Jul 5, 2:32 pm, SirG <[e-mail]:

Glad to help (and procrastinate on my project.)

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?

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

[Dr J R Stockton]

In comp.lang.javascript message <11*********************@r34g2000hsd.goo
glegroups.com>, Thu, 5 Jul 2007 14:39:54, SirG
<st************@gmail.composted:

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

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"

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

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

>What am I doing wrong?

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

--
(c) John Stockton, Surrey, UK. ?@merlyn.demon.co.uk Turnpike v6.05 IE 6
news:comp.lang.javascript FAQ <URL:http://www.jibbering.com/faq/index.html>.
<URL:http://www.merlyn.demon.co.uk/js-index.htmjscr maths, dates, sources.
<URL:http://www.merlyn.demon.co.uk/TP/BP/Delphi/jscr/&c, FAQ items, links.

[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);

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.

What is the more correct way of doing this?

[SirG]

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

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.

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

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)

What am I doing wrong?

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. ?[e-mail] Turnpike v6.05 IE 6
news:comp.lang.javascript FAQ <URL:http://www.jibbering.com/faq/index.html>.
<URL:http://www.merlyn.demon.co.uk/js-index.htmjscr maths, dates, sources.
<URL:http://www.merlyn.demon.co.uk/TP/BP/Delphi/jscr/&c, FAQ items, links.

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.

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

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)

[-Lost]

SirG wrote:

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

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.

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

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)

>>What am I doing wrong?

Copying bad code.

Agreed.

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

<snipped "Sir Stockton" signature>

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.

Great job actually listening and learning something! Remember, you
don't need to quote anyone's signature as well.

Where is "rf?" He'd be proud.

--
-Lost
Remove the extra words to reply by e-mail. Don't e-mail me. I am
kidding. No I am not.