0

I'm having a problem with the following bit of code and I'm wondering what I'm doing wrong.

I'm getting an Uncaught SyntaxError: Unexpected token ) error on the line where the eval call is made.

function myfunction(){
    var p1 = '';
    var p2 = '';
    var p3 = '';

    for (i=1; i<=3; i++){
        eval("$('#p"+i+"').closest('.filter').find('.vals div').each(function(){if ($.trim(p"+i+").length > 0) {p"+i+" += ',';} p"+i+" += $(this).attr('class');});");
    }
}

Here is the applicable HTML:

<div class="filter">
    <label>Organizations</label>
    <input id="p1" type="text" value="" />
    <div class="vals">
        <div class="3" title="Click to remove">ABC School District</div>
        <div class="4" title="Click to remove">DEF School District</div>
    </div>
</div>
<div class="filter">
    <label>Groups</label>
    <input id="p2" type="text" value="" />
    <div id="vals"></div>
</div>

If it isn't completely obvious I'm also using jQuery here.

Thanks

2
  • 5
    "I'm wondering what I'm doing wrong" -- My answer would be using eval() in the first place ... You have a hard to debug statement. I don't see why you can't write this without eval() ... Commented Oct 26, 2011 at 13:22
  • You can skip using eval for statements like these "$('#p"+i+"')... by simply using $('#p['+i) as jquery takes the selecter as string. I really do not see any advantage of eval here. Commented Oct 26, 2011 at 13:30

3 Answers 3

5

You are better off using an array and just executing the code normally:

function myfunction(){
    var p  = [null, '', '', '']; //Empty zeroth element to keep your 1-indexing

    for (i=1; i<=3; i++){
        $('#p'+i).closest('.filter').find('.vals div').each(function(){
            if ($.trim(p[i]).length > 0) {
                p[i] += ',';
            }
            p[i] += $(this).attr('class');
        });
    }
}
Sign up to request clarification or add additional context in comments.

4 Comments

dammit! +1 for beating me by a minute!
Huh? Totally incorrect! ES5 10.4.2 eval in most scenarios runs in local scope (and in ES3 I think it's in every case).
@davin Yea, my test was poorly constructed/I was thinking of setTimeout. Still recommend the removal of eval though.
@Dennis: Perfect... Thank you for actually reading through and taking time to understand the purpose of the code (sorry for it being hard to read) and offering up a better solution.
4

Your code, no eval

function myfunction(){
    var p1 = '';
    var p2 = '';
    var p3 = '';

    for (i=1; i<=3; i++){
        $("#p"+i).closest(//..... rest here

2 Comments

Nope, I'm afraid that won't work. I'm trying to assign values to differently named variables in my eval();
You should use an array instead of using different variables when looping
1

You might be missing a "

 eval("$('#p" + i + "'")...

FWI: Instead of nesting functions and chaining, you'll find it much easier to break it into several discrete lines of code and use named functions.

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.