Eval this!

sproketboy

While reviewing some old JavaScript code in our application, I came across this pain.

function selectCompany(companyName,nNo,cMask) {
   companyName = unescape(companyName);
   if (window.opener){
      jStr1 = "window.opener.document.all.IcompanyName.value = trimString(unescape(companyName))"
      jStr2 = "window.opener.document.all.IcompanyNo.value = nNo"
      jStr3 = "window.opener.document.all.PolicyNumber.mask = cMask"
      jStr5 = "window.opener.document.all.PolicyNumber.title = cMask"
      jStr4 = "window.opener.document.all.IcompanyName.reset()"
      eval(jStr1)
      eval(jStr2)
      try {eval(jStr3)}catch(e){}
      try {eval(jStr4)}catch(e){}
      try {eval(jStr5)}catch(e){}
      if(window.opener.RefreshListByInsurer){
         window.opener.execScript(window.opener.RefreshListByInsurer(nNo,cMask));
      }
   }
   window.close()
}

For some reason the developer (and I use that term very loosely) has decided that he has to EVAL each line of code. Some in a try/catch with nothing in the catch. I guess he never heard of using an IF to check if the object in question exists first.

This is where my gray hair comes from. :(

ammoQ

moved to "Side Bar" WTF

Pap

Now move to CodeSOD :)

mrprogguy

Never mind that the lines themselves (should one ever attempt to do this) could just be wrapped in a try block without all that eval() mess.

Anyone notice that unescape(companyName) is done twice?

sproketboy

The pain continues.

if(window.opener.RefreshListByInsurer){
window.opener.execScript(window.opener.RefreshListByInsurer(nNo,cMask));
}

Why is he executing this function thru execScript?

it should be:

if(window.opener.RefreshListByInsurer){
window.opener.RefreshListByInsurer(nNo,cMask);
}

I hate JavaScript with a passion.

I feel your pain. I suspect that this is his way of making errors go away. Probably not a bad idea, if we were still slapping "Best viewed in XYZ!" on the bottom of our websites.

FYI, document.all is a nono. use document.getElementById('myElementId') instead.

You may find this useful: http://developer.mozilla.org/en/docs/Migrate_apps_from_Internet_Explorer_to_Mozilla

There are actually cases where eval is useful, but most of them involve things like:


var myString = "foo";
var yourString = "bar";
var whose = "my";
eval(whose + "String = baz");

and as such tend to be rather rare, especially given the fact that every object is a hash of sorts, so myObj[whose + "String"] would work the same as myObj.myString

dhromed

Knew a developer who did:

for (loop over some array using i) {

eval("var smurf" + i + " = " + array[i]);

}

I set him straight real quikclike.

sproketboy

Thanks for the link!

Yeah, unfortunately the original developers were M$ kool-ade drinkers. There are many IE bits in the app. Every version I push to get the UI rewritten but so far we haven't had a client push for it. Though we have lost a few prospects because they were using Linux/Mac machines.

asuffield

@sproketboy said:

Thanks for the link!
Yeah, unfortunately the original developers were M$ kool-ade drinkers. There are many IE bits in the app. Every version I push to get the UI rewritten but so far we haven't had a client push for it. Though we have lost a few prospects because they were using Linux/Mac machines.

Clients never push for things like this. They all behave the same way you have observed: if you haven't already bothered to port your application, then there are plenty of saner companies they could buy stuff from instead. They won't see it as worth their time to even explain their requirements to you.

PSWorx

@Volmarias said:

I feel your pain. I suspect that this is his way of making errors go away. Probably not a bad idea, if we were still slapping "Best viewed in XYZ!" on the bottom of our websites.

FYI, document.all is a nono. use document.getElementById('myElementId') instead.

You may find this useful: http://developer.mozilla.org/en/docs/Migrate_apps_from_Internet_Explorer_to_Mozilla

There are actually cases where eval is useful, but most of them involve things like:
var myString = "foo"; var yourString = "bar"; var whose = "my"; eval(whose + "String = baz");
and as such tend to be rather rare, especially given the fact that every object is a hash of sorts, so myObj[whose + "String"] would work the same as myObj.myString

There is exactly one valid use of eval() and that is parsing JSON response data from AJAX requests. Everything else is a clear sign that either the programmer or the designer has stuff to learn.

iwpg

@PSWorx said:

There is exactly one valid use of eval() and that is parsing JSON response data from AJAX requests. Everything else is a clear sign that either the programmer or the designer has stuff to learn.

How would you write this without eval()?

    var params = [];
    for (var i = 0; i < args.length; i++)
        params.push("args[" + i + "]");
    var code = "new c(" + params.join(", ") + ")";
return k(eval(code));</pre></blockquote>

Kemp

First you explain in english what it's meant to do, then we tell you why it's simple without eval :-P

asuffield

@Kemp said:

First you explain in english what it's meant to do, then we tell you why it's simple without eval :-P

It's a simple form of metaprogramming, and javascript doesn't have any metaprogramming features other than eval. His point being that any form of higher-level programming in javascript will have to be based on eval.

Kemp

Well my point was going to be about people choosing a solution (in this case "a form of metaprogramming") and trying to make it work for their problem, rather than choosing a suitable solution based on their problem. The implemented method of working may not be possible in javascript, but can the overall goal of the code be achieved without required that? (In which case a direct conversion of that code is pointless as it isn't needed anymore)

In case it wasn't obvious, I know small amounts of javascript but nowhere near enough enough to form an authoritive decision on anything here, so I'm speaking in the general sense rather than going into specifics.

masklinn

@sproketboy said:

The pain continues.

if(window.opener.RefreshListByInsurer){
         window.opener.execScript(window.opener.RefreshListByInsurer(nNo,cMask));
}

Why is he executing this function thru execScript?
it should be:
if(window.opener.RefreshListByInsurer){

         window.opener.RefreshListByInsurer(nNo,cMask);

}

I hate JavaScript with a passion.

Except it's not a problem with javascript, it's a problem with the person who wrote the script.

@iwpg said:

@PSWorx said:

There is exactly one valid use of eval() and that is parsing JSON response data from AJAX requests. Everything else is a clear sign that either the programmer or the designer has stuff to learn.

How would you write this without eval()?

    var params = [];
    for (var i = 0; i < args.length; i++)
        params.push("args[" + i + "]");
    var code = "new c(" + params.join(", ") + ")";
return k(eval(code));</pre></blockquote></blockquote>

You wouldn't write it at all.
@asuffield said:@Kemp said:
First you explain in english what it's meant to do, then we tell you why it's simple without eval :-P
It's a simple form of metaprogramming, and javascript doesn't have any metaprogramming features other than eval.
It's not metaprogramming.
@asuffield said:His point being that any form of higher-level programming in javascript will have to be based on eval.

Wha?
Are you joking?
The only thing that happens here is using an array as the arglist of a constructor, which is stupid in the first place (how about just passing and retrieving an array?), it's not "higher-level" whatever that may mean, and it's not anywhere close to metaprogramming.

asuffield

@masklinn said:

@asuffield said:
@Kemp said:
First you explain in english what it's meant to do, then we tell you why it's simple without eval :-P
It's a simple form of metaprogramming, and javascript doesn't have any metaprogramming features other than eval.

It's not metaprogramming.

It's a piece of code that generates and executes similar-but-different code based on input data. That's metaprogramming.

@asuffield said:
His point being that any form of higher-level programming in javascript will have to be based on eval.

Wha?

Are you joking?

The only thing that happens here is using an array as the arglist of a constructor, which is stupid in the first place (how about just passing and retrieving an array?), it's not "higher-level" whatever that may mean, and it's not anywhere close to metaprogramming.

Higher-level programming is any form of programming that manipulates programs (level N being a program that manipulates a level N-1 program; metaprogramming is generally considered to be the second order); it's a standard concept in semantics theory, and has its roots in the theories of logic and computable functions.

It is traditional when demonstrating programming concepts to use simple examples. Every example in any textbook you look at is very probably a "stupid" example that nobody would ever actually write, because it is only there to demonstrate one particular concept. This one is no different. Real-world examples are usually too long and boring to work with (any practical example of metaprogramming is likely to take up the better part of a page, rather than a few lines).

The point remains that javascript's only capacity for anything like this is eval.

iwpg

@Kemp said:

The implemented method of
working may not be possible in javascript, but can the overall goal of
the code be achieved without required that? (In which case a direct
conversion of that code is pointless as it isn't needed anymore)

The code comes from the run-time support library of a JavaScript preprocessor that I wrote. Among other things, it needs to add special handling for new expressions, if the constructor is a particular sort of object, otherwise it uses a regular new. Since JavaScript is dynamic, the choice has to be made at runtime (it might be possible to choose one or the other at preprocessing-time in some cases, but I didn't attempt anything like that, and it wouldn't work in general). There are two possible ways to do that: one would be to inline the logic into the generated code. This has the advantage that the generator would know how many arguments need to be passed, so no need for eval tricks, but it would be rather bloaty. The alternative would be to make the generator insert a call to a library function, which is what I did. The snag is that the library function needs to handle any number of constructor arguments, and as far as I know that requires eval. Some people might prefer the eval-less version, but I'd say that's a matter of taste.

@masklinn said:

You wouldn't write it at all.

Well, clearly I did write it. :-P Admittedly it's rather an edge-case, though.

@masklinn said:

how about just passing and retrieving an array?

That only works if you get to define the contructor.

@mrprogguy said:

Never mind that the lines themselves (should one ever attempt to do this) could just be wrapped in a try block without all that eval() mess.

Anyone notice that unescape(companyName) is done twice?

No one can escape companyName! Muahahahaha!

dhromed

@iwpg said:

That only works if you get to define the contructor.

In javascript, all that distinguishes a method call from an object are the two parentheses. So you do get to define the constructor, by simply passing it. Hell, add a property to the argument array that contains the constructor that is supposed to take that array as an argument.

In addition to that, members can be called by literal and by string ( the ever-lovable thing["methodname"] syntax ), and you can tack parens to the end of that ( thingmydynamicmethodname ), so JS, at first glance, provides enough methods for "dynamic programming" that make the use of eval redundant.

The snag is that the library function needs to handle any number of
constructor arguments, and as far as I know that requires eval.

Any function automatically instantiates an arguments array, which contains all arguments passed to a call. A function that needs a variable amount of arguments can suffice with not having any formal arguments defined at all, and strictly rely on that arguments array.

Some
people might prefer the eval-less version, but I'd say that's a matter
of taste.

eval incurs something of an execution time cost, and may under certain circumstances open up a security hole similar to SQL injection. No to mention the moist gurgle that keeps ringing in your ears as maintainability is sucked down the drain.

I acknowledge that there are problems best solved with eval. But:
A) think HARD before you resort to it.
B) usually, the use of eval is the result of a program design error.

Random832

@iwpg said:

@PSWorx said:
There is exactly one valid use of eval() and that is parsing JSON response data from AJAX requests. Everything else is a clear sign that either the programmer or the designer has stuff to learn.

How would you write this without eval()?
    var params = [];
    for (var i = 0; i < args.length; i++)
        params.push("args[" + i + "]");
    var code = "new c(" + params.join(", ") + ")";

    return k(eval(code));

That's interesting - normally, you could use .apply, but you can't do that with a constructor. If not for the use of 'new', you could just do fn.apply(null,args)

iwpg

@dhromed said:

@iwpg said:
That only works if you get to define the contructor.
In javascript, all that distinguishes a method call from an object are the two parentheses. So you do get to define the constructor, by simply passing it. Hell, add a property to the argument array that contains the constructor that is supposed to take that array as an argument.

Not sure what you mean here. I think masklinn was suggesting that, if the number of arguments to the constructor needs to change dynamically, that it should be defined to accept an array. My point was that that's not possible if the constructor is already defined elsewhere (built into the browser, for example).

The snag is that the library function needs to handle any number of
constructor arguments, and as far as I know that requires eval.
Any function automatically instantiates an arguments array, which contains all arguments passed to a call. A function that needs a variable amount of arguments can suffice with not having any formal arguments defined at all, and strictly rely on that arguments array.

Right, but that's on the receiving end. To [i]call[/i] a function with a variable number of arguments, there's .apply(), but nothing similar (as far as I know) to invoke it as a constructor, rather than a regular function.

(My code does indeed use .apply() for regular calls, but I'm actually reconsidering that... the apply method can be replaced at runtime, just like any other property, and if the "function" is a host object it may never have had an apply() method in the first place, despite being callable. Of course, the vast majority of people doing JavaScript don't have to worry about such shenanigans.)

eval incurs something of an execution time cost

True in general, but worrying about speed in this particular application is pretty futile, eval() or no. ;-)

and may under certain circumstances open up a security hole similar to SQL injection. No to mention the moist gurgle that keeps ringing in your ears as maintainability is sucked down the drain.

Again, valid points, but I'm pretty confident that in this case I'm safe.

I acknowledge that there are problems best solved with eval. But:
A) think HARD before you resort to it.
B) usually, the use of eval is the result of a program design error.

Fair enough.

Howi

@iwpg said:

@PSWorx said:
There is exactly one valid use of eval() and that is parsing JSON response data from AJAX requests. Everything else is a clear sign that either the programmer or the designer has stuff to learn.

How would you write this without eval()?
    var params = [];
    for (var i = 0; i < args.length; i++)
        params.push("args[" + i + "]");
    var code = "new c(" + params.join(", ") + ")";
return k(eval(code));</PRE></BLOCKQUOTE>
I may have misinterpereted what your code is supposed to do but I believe it's meant to make a new c, with dynamic params (like those passed to the calling function)...

var indexes = [];
for ( var i=0; i<params.length; i++ ) {
indexes[i] = i;
}
var funcStr = "return new cFunction(params[" + indexes.join( "], params[" ) + "]);";
return new Function( "params", "cFunction", funcStr )( params, c );

where c is the function that instantiates your new object. Note that instead of putting 'cFunction' in and passing c as a parameter, you could just use 'c' in the string, but this allows you to pass around function references like a demon, without worrying about the scope (not that there SHOULD be a problem, but it scares some people). Please note that the code above is practically an eval, but it's not an eval :)

iwpg

@Howi said:

I may have misinterpereted what your code is supposed to do but I believe it's meant to make a new c, with dynamic params (like those passed to the calling function)...

var indexes = [];
for ( var i=0; i<params.length; i++ ) {
indexes[i] = i;
}
var funcStr = "return new cFunction(params[" + indexes.join( "], params[" ) + "]);";
return new Function( "params", "cFunction", funcStr )( params, c );

where c is the function that instantiates your new object. Note that instead of putting 'cFunction' in and passing c as a parameter, you could just use 'c' in the string, but this allows you to pass around function references like a demon, without worrying about the scope (not that there SHOULD be a problem, but it scares some people). Please note that the code above is practically an eval, but it's not an eval :)

Yes, I think that would work, thanks. As you say, though, it's close enough to eval() that I expect most of the objections would still hold.