eval?

[OBCENEIKON]

I am getting an error on a script that is used to hide and show a div layer.

It works great in Firefox but I get an error in IE.

Line: 500
Char: 3
Error: Expected ';'
Code: 0
URL: http://plurpage.com/everquest/dontest.php

Here is that specific block of code

var state = 'none';

function showhide(layer_ref) {

if (state == 'block') {
 state = 'none';
} else {
 state = 'block';
}

if (document.all) { //IS IE 4 or 5 (or 6 beta)
 eval("document.all."+ layer_ref +".style.display = state");
}
if (document.layers) { //IS NETSCAPE 4 or below
 document.layers[layer_ref].display = state;
}
if (document.getElementById &&!document.all) {
 hza = document.getElementById(layer_ref);
 hza.style.display = state;
}
}

line 500 is the eval line...I am by far no JS guru so lets keep that in mind when replying :P

[jscheuer1]

It is hard to just jump into a script and say how a particular line should be rewritten. And I see you've since changed the offending line to:

eval("document.all."+ layer_ref +".style.display = '" + state + "';");

which looks like it would have a better chance of working than what you had but, strikes me as more complex than it needs to be. I also see there is now a new error, on line 660:

this.obj = document.all[id].style;

I always get confused with this syntax, sometimes you need to precede the line with:

id=id;

sometimes the syntax is:

this.obj = document.all['id'].style;

and sometimes it is:

this.obj = document.all.id.style;

Good Luck!

[mwinter]

Here is that specific block of code

I really dislike this function, so I'll provide an alternative later. At a quick glance, there is nothing wrong as such with this code, though the unnecessary use of eval will cause problems in some situations. I'll cover that when I get there.

The current problem with the code at your site is that you call layerObject without providing an expected argument, id. You don't seem to be using showhide anywhere.

I'm curious to know why that document is so heavily scripted. You apparently have the ability to process information on the server, so why rely so much on the client? The client is unreliable. The server isn't.

var state = 'none';

Using a global for a function that could be called for multiple elements is a really stupid idea. If your hidden elements default to 'none' in a style sheet rule, you can easily determine that from the style object. The following example omits feature detection for clarity.

var state;

if('block' == elementRef.style.display) {
  state = 'none';
} else { /* 'none' or an empty string */
  state = 'block';
}

Or, more compactly:

var state = ('block' == elementRef.style.display)
          ? 'none' : 'block';

if (state == 'block') {
state = 'none';
} else {
state = 'block';
}

As evidenced above, I think it better to write that using the conditional operator (<expr> ? <expr> : <expr>).

if (document.all) { //IS IE 4 or 5 (or 6 beta)

Incorrect. That would also evaluate to include Opera and IceBrowser, amongst others. Use this sort of thing to determine what a user agent supports, not what it is. The former matters. The latter doesn't.

eval("document.all."+ layer_ref +".style.display = state");

The problem with this eval statement is what if layer_ref contains 'invalid' characters, such as dots (.), hyphens (-), and so on? The host will then evaluate code such as

document.all.id-5.style.display = state;

which would be interpreted as, "assign state to 5.style.display and subtract that from document.all.id", which is complete nonsense.

The far superior alternative is:

document.all[layer_ref].style.display = state;

which will never suffer the previous problem.

}
if (document.layers) {

These should really be a series of exclusive code branches. If a user agent took one of the previous routes, why should it take another one?

So, the replacement:

function toggleDisplay(element) {var style;
  if('string' == typeof element) {
    if(document.getElementById) {
      element = document.getElementById(element);
    } /* else if(document.all) {
      element = document.all[element];
    } else if(document.layers) {
      element = document.layers[element];
    } */
  }
  if(element) {
    style = element.style || element;
    style.display = ('block' == style.display)
                  ? 'none' : 'block';
  }
}

I've effectively removed the document.all and document.layers branches as they shouldn't really be necessary any more. If you want to keep them, remove the comment delimiters.

I could take this function further, but it will probably cater for your needs.

If none of this helps, please post a URL to a simple demonstration that shows the problem clearly.

Mike

[OBCENEIKON]

WOW...very very helpful, I may not be a JS guru, but with tips like that it wont be long :P

Thanks so much, as you may have guessed, this was a prewritten script I found on a messageboard.

I'm curious to know why that document is so heavily scripted. You apparently have the ability to process information on the server, so why rely so much on the client? The client is unreliable. The server isn't.

That was actually something I have been contemplating, and considering changing to fix another problem...gonna take a look into that.

Again thank you, oh, and if you do have a better script to acheive a show/hide layer in the same onclick function, I would appreciate it...most show/hide scripts require two functions, one to show and one to hide....resulting in two links

[mwinter]

Thanks so much, as you may have guessed, this was a prewritten script I found on a messageboard.

You can find all kinds of junk on the Web. Generally, any code that contains an eval function call is of dubious quality, as eval has very specific usage but is often abused as some kind of panacea.

Again thank you

You're welcome.

if you do have a better script to acheive a show/hide layer in the same onclick function, I would appreciate it

The script as written will work as a toggle (hence the name ). In principle it could be improved by using computed style information to determine which values to set, but it seems that Konqueror and Safari don't support this. I'll have to investigate.

Mike