Specific lines in javascript function not working

Question

I just got this function working this evening. However, after a few minor edits getting a different part of the if/else to go, part of it has magically stopped working.

The four for loops in the callback function of the json request all execute, but none of the DOM manipulations are actually made. I've triple checked that the appropriate DOM elements exist for this to happen. Alerts will fire in all of them. Just the jQuery lines are failing.

I have tried putting the relevant variables into the console and manually iterating through the numbers to simulate a loop. This works fine. I've also used alerts to display the sequence of the variables in the loop, and these are all functioning properly.

I'm baffled.

function drawPreview() {
    var $preview = $('#preview');
    $preview.remove();
    $('#general_preview').remove();
    try {
        activecell.location;
    }
    catch (error) {
        $('#active_cell').clone().attr('id','general_preview').appendTo('#win_preview');
        return;
    }
    if (activecell.location.match(/^\d+_\d+$/)!==null) {
        var x = parseInt(activecell.location.slice(0,activecell.location.indexOf("_")));
        var y = parseInt(activecell.location.slice(activecell.location.indexOf("_")+1));
        var area = "x"+activearea.join("x")+"x";
        $('#win_preview').append($('<div id="preview"><div><div></div><div></div><div></div></div><div><div></div><div></div><div></div></div><div><div></div><div></div><div></div></div></div>'));
        var i = y-1;
        var j = x-1;
        function loadCell() {
            var exp = new RegExp("x"+i+"_"+j+"x","g");
            if (area.match(exp)) {
                if (i==y&&j==x) {
                    $('#active_cell').clone().children().unwrap().appendTo($preview.children().eq(1).children().eq(1));
                    ++j;
                    loadCell();
                }
                else {
                    var jqxhr = $.getJSON('data/areas/'+$('#select_area').val()+'/'+x+"_"+y+'.json', function(data) {
                        var tmp = data;
                        for (var l=0; l<9; ++l) {
                            $preview.children().eq(i-y+1).children().eq(j-x+1).append('<div></div>');
                        }
                        for (var l=0; l<9; ++l) {
                            $preview.children().eq(i-y+1).children().eq(j-x+1).children().append('<div></div>');
                        }
                        for (var l = 0; l < 9; ++l) {
                            for (var m = 0; m < 9; ++m) {
                                $preview.children().eq(i-y+1).children().eq(j-x+1).children().eq(l).children().eq(m).attr("style","background: #"+tmp.p.c[tmp.c[l][m]-1]+" url(textures/terrain/"+tmp.p.t[tmp.t[l][m]-1]+".png) bottom center no-repeat");
                            }
                        }
                        if (i==y+1&&j==x+1) {
                            return;
                        }
                        else if (j==x+1) {
                            ++i;
                            j = x-1;
                            loadCell();
                        }
                        else {
                            ++j;
                            loadCell();
                        }
                    })
                        .error(function() { alert("There was an error loading the data. The data may be invalid or you may be looking for a file that does not exist."); })
                }
            }
            else {
                if (i==y+1&&j==x+1) {
                    return;
                }
                else if (j==x+1) {
                    ++i;
                    j = x-1;
                    loadCell();
                }
                else {
                    ++j;
                    loadCell();
                }
            }
        }
        loadCell();
    }
}

" wrote out this entire function without testing it..." - :-( You may want to go back and work on that part first. — JasCav
– JasCav, Commented Mar 13, 2011 at 5:40
I'm sorry, if you actually read this you would know that I did get it working. — jsonnull
– jsonnull, Commented Mar 13, 2011 at 5:42
@JasonSage - I did read the whole thing. You said (before your edit), "I wrote out this entire function without testing it and I got it working this evening." That's cool and all, but if you have no tests, then how do you ever expect to debug it? There's not even a good starting point. That's what I meant with my original comment. Wasn't trying to be a dick, but, I always like to encourage good development practices as it helps prevent situations like that one you're in now (AKA completely baffled). — JasCav
– JasCav, Commented Mar 13, 2011 at 5:49
I expect to debug it exactly the way I did, by picking out the parts that weren't working and rewriting them correctly. This function had a lot going on and I wanted to get the structure down before getting bogged down in the details. Either way, I doubt it would have prevented the situation I am in right now, since the problem has arisen since I got it working. Now unless you're completely baffled as well, I'd appreciate some help instead of useless criticism on how I chose to develop this function. — jsonnull
– jsonnull, Commented Mar 13, 2011 at 6:02
@gabriel looks like he's setting up his cells. how would you accomplish that?! — philwinkle
– philwinkle, Commented Mar 13, 2011 at 6:54

rdamborsky · Accepted Answer · 2011-03-13 07:39:59Z

3

at the beginning, you're calling this

var $preview = $('#preview');

then, in the first if branch:

$('#win_preview').append($('<div id="preview"><div...

which is ok, but in callback, by doing

$preview.children().eq(i-y+1).children()....

you're trying to access element that doesn't exist anymore. You need to get new reference to your just appended #preview. Maybe after that line with tons of <div></div> appends :)

EDIT:

Also, I'd recommend you to use a little bit of class selectors, instead of relying on exact DOM tree structure (using lots of eq()). It may save you some time in the future if you want to update markup somehow. Also, once you get into styling of this, you could get these selectors as a CSS styling side-effect, so why not to use them right away.

Another thing: if code gets this complex and not much readable, you can try to help yourself with a bit of debugging:

for (var l=0; l<9; ++l) {
    $preview.children().eq(i-y+1).children().eq(j-x+1).append('<div></div>');
}

could be also wrote as

var targetParent = $preview.children().eq(i-y+1).children().eq(j-x+1);
console.log(targetParent);
for (var l=0; l<9; ++l) {
    targetParent.append('<div></div>');
}

you'll see in the console (assuming firefox+firebug debugging), where exactly you're trying to append to. Added benefit of this is that it forwards you to optimization of your code - you're getting append target once and then just appending to it in the loop. Previous variant would do both getting target and appending in the loop.

edited Mar 13, 2011 at 7:39

answered Mar 13, 2011 at 7:29

rdamborsky

1,9301 gold badge16 silver badges22 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

jsonnull Over a year ago

Wow I bet that does it. Also, regarding the class selector, I'm not sure there is any benefit in using them. It is already styled according to the number of nested div elements and I will only be accessing them in specific numeric order. That said, I'm sure I should store a reference to the elements I'm refusing in the loops. Thanks a bunch for (hopefully) clearing this up!

Gabriel · Accepted Answer · 2011-03-13 07:35:49Z

I have done a fairly standard refactor to reduce the number of nested if statements (this is a common breeding ground for logic errors and confusion). I would encourage you to post a bit of test data ( a fixture if you will ) of the data you would expect to get this actually working. A segment of valid dom and a sample of cell data should be enough.

function drawPreview() {
    var $preview = $('#preview'),
        x, y, area, i, j;

    function repeat( text, count ){
        var i=0, out = '';
        for( ; i++ < count ; ) {
            out += text;
        }
        return out;
    }

    $preview.remove();
    $('#general_preview').remove();
    try {
        activecell.location;
    }
    catch (error) {
        $('#active_cell').clone().attr('id','general_preview').appendTo('#win_preview');
        return;
    }
    if (activecell.location.match(/^\d+_\d+$/) === null ){
        return;
    }
    x = parseInt(activecell.location.slice(0,activecell.location.indexOf("_")), 10);
    y = parseInt(activecell.location.slice(activecell.location.indexOf("_")+1), 10);
    area = "x"+activearea.join("x")+"x";
    $('#win_preview').append($('<div id="preview"><div>' + repeat('<div>' + repeat('<div></div>', 4) + '</div>', 3) + '</div></div>'));
    i = y-1;
    j = x-1;
    function loadCell() {
        var exp = new RegExp("x"+i+"_"+j+"x","g"),
            sel_area;

        if (! area.match(exp)){
            if (i==y+1&&j==x+1) {
                return;
            }
            else if (j==x+1) {
                ++i;
                j = x-1;
                loadCell();
            }
            else {
                ++j;
                loadCell();
            }
            return;
        }
        if (i==y&&j==x) {
            $('#active_cell').clone()
                .children()
                .unwrap()
                .appendTo($preview.children()
                                .eq(1)
                                .children()
                                .eq(1)
                );
            ++j;
            loadCell();
            return;
        }
        var sel_area = $('#select_area').val();
        var jqxhr = $.getJSON('data/areas/'+ sel_area +'/'+x+"_"+y+'.json', function(data) {
            var tmp = data, l = 0, m = 0;
            for ( l=0; l<9; ++l ) {
                $preview.children()
                        .eq(i-y+1)
                        .children()
                        .eq(j-x+1)
                        .append('<div></div>');
            }
            for ( l=0; l<9; ++l) {
                $preview.children()
                        .eq(i-y+1)
                        .children()
                        .eq(j-x+1)
                        .children()
                        .append('<div></div>');
            }
            for ( l = 0; l < 9; ++l) {
                for ( m = 0; m < 9; ++m) {
                    $preview.children()
                            .eq(i-y+1)
                            .children()
                            .eq(j-x+1)
                            .children()
                            .eq(l)
                            .children()
                            .eq(m)
                            .css({background: "#"+tmp.p.c[tmp.c[l][m]-1]+" url(textures/terrain/"+tmp.p.t[tmp.t[l][m]-1]+".png) bottom center no-repeat"});
                }
            }
            if (i==y+1&&j==x+1) {
                return;
            }
            else if (j==x+1) {
                ++i;
                j = x-1;
                loadCell();
                return;
            }
            ++j;
            loadCell();
            return;
        }).error(function() { alert("There was an error loading the data. The data may be invalid or you may be looking for a file that does not exist."); });
    }
    loadCell();
}

Collectives™ on Stack Overflow

Specific lines in javascript function not working

2 Answers 2

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related