8
\$\begingroup\$

I have 2 tables. One holds only first column of my "data table" rest is stored in another table, that is placed right to first. What I need to do is to get all the data from specific row to pass it to jqplot.

My tables look like this. My code basically works, but I think it can be improved.

$('table#baseTable  > tbody > tr > td').click(function() {
    var rowIndex = $(this).parent().index();
    $('div#log').html(rowIndex);

    var myData = [];

    $('#dataTable tbody tr:eq(' + rowIndex + ')').map(function() {
            return $(this.cells).get();
        }).each(function() {
    var headerVal = $(this).closest("table").find("thead > tr > th").eq($(this).index()).html();
    myData.push([headerVal, $(this).html()]);
})

    alert(myData);
    console.log(myData);
});​

I'm using this code in ASP page, so that every time I use UpdatePanel I must call my functions again.

This is my plot function:

function plot() {
    var $plot;
    var dataTableRows = $('#Grid3_br tbody tr');

    $('td.akcje').on('click', function() {
        var $newTitle = $(this).next().text();
        var myData = [],
            element = $(this),
            rowIndex = element.parent().index();

        $(dataTableRows[rowIndex]).map(function() {
            return $(this.cells).get();
        }).each(function(index, value) {
            myData.push(['M' + index, parseFloat($(this).html())]);
        })
        $('#wykres1').empty();//clears old plot. Don't know why re-plotting don't work for me
        $plot = $.jqplot('wykres1', [myData], {
            title: $newTitle,
            axes: {
                xaxis: {
                    renderer: $.jqplot.CategoryAxisRenderer,
                    tickOptions: {
                        formatString: '%s'
                    }
                },
                yaxis: {
                    tickOptions: {
                        formatString: '%.2f zł'
                    }
                }
            },
            highlighter: {
                show: true,
                sizeAdjust: 7.5,
                tooltipAxes: 'y'
            },
            cursor: {
                show: false
            }
        });

        $(window).resize(function() {
            $plot.replot({
                resetAxes: true
            });
        });
    });
}
\$\endgroup\$
9
\$\begingroup\$

I'll start with your selectors:

First off, you should never have a selector with anything left of an id - searches for ids are about the fastest you can get on the DOM.

Second, if you don't want to wrap other tables in your tables (I assume you don't), get rid of the immediate child stuff (P > C). Searching by tag is also pretty fast, so we use the id-element as our base for that.

$('#baseTable tbody td').click(function() {
    var rowIndex = $(this).parent().index();
    $('#log').html(rowIndex);

    var myData= [];

    $($('#dataTable tbody tr')[rowIndex]).map(function() {
        return $(this.cells).get();
    }).each(function() {
        var headerVal = $(this).closest('table').find('thead th')
            .eq($(this).index()).html();
        myData.push([headerVal,parseFloat($(this).html())]);
    })

    alert(myData);
    console.dir(myData);
});​

Next, you should cache elements you found, use more than once and which don't change. And usually, it's a bad choice to bind events to the elements directly. I'm assuming jQuery 1.7 is ok and you don't really want to use jQuery 1.4, so we can use on for event binding.

var logDiv = $('#log'),
    dataTableRows = $('#dataTable tbody tr');

$('#baseTable tbody').on('click', 'td', function() {
    var myData = [],
        element = $(this),
        rowIndex = element.parent().index();

    logDiv.html(rowIndex);

    $(dataTableRows[rowIndex]).map(function() {
        return $(this.cells).get();
    }).each(function() {
        var headerVal = $(this).closest('table').find('thead th')
            .eq($(this).index()).html();
        myData.push([headerVal,parseFloat($(this).html())]);
    })

    alert(myData);
    console.dir(myData);
});​

Next, accessing the DOM is rather slow. If you need the data for jqplot, do you need the table headers and can the data change?

Anyway, I'd store all data in an array beforehand. Now it looks like this:

var logDiv = $('#log'),
    dataTable = $('#dataTable'),
    dataTableHeaders = [],
    dataTableData = [];

// cache data table headers
dataTable.find('thead th').each(function(col) {
    dataTableHeaders[col] = $(this).html();
});

// cache full row (including headers) for data table
dataTable.find('tbody tr').each(function(row) {
    var rowData = (dataTableData[row] = []);
    $(this).children().each(function(col) {
        rowData[2 * col] = dataTableHeaders[col];
        rowData[2 * col + 1] = parseFloat($(this).html());
    });
});

// the onClick - but on tr instead of td and with a fast lookup
$('#baseTable tbody').on('click', 'tr', function() {
    var rowIndex = $(this).index(),
        myData = dataTableData[rowIndex];
    logDiv.html(rowIndex);
    alert(myData);
    console.dir(myData);
});​

I still don't like the need for index(). I like it even less than event binding on elements, so I'll compromise:

var logDiv = $('#log'),
    dataTable = $('#dataTable'),
    dataTableHeaders = [],
    dataTableData = [];

dataTable.find('thead th').each(function(col) {
    dataTableHeaders[col] = $(this).html();
});

dataTable.find('tbody tr').each(function(row) {
    var rowData = (dataTableData[row] = []);
    $(this).children().each(function(col) {
        rowData[2 * col] = dataTableHeaders[col];
        rowData[2 * col + 1] = parseFloat($(this).html());
    });
});

$('#baseTable tbody tr').each(function(row) {
    $(this).on('click', function() {
        var data = dataTableData[row];
        logDiv.html(row);
        alert(data);
        console.dir(data);
    });
});

​Here's my last step (this is not really needed, but I like it): we isolate this code from the outside world and make accidental messups a lot harder. And we enable a recalculation and rebinding of the click event if anything changes (number of rows / cols or the content).

var tableData = (function(){
    var logDiv = $('#log'),
        cacheRows = function() {
            var dataTable = $('#dataTable'),
                dataTableHeaders = [],
                dataTableData = [];
            // fetch headers
            dataTable.find('thead th').each(function(col) {
                dataTableHeaders[col] = $(this).html();
            });
            // fetch column data, prepare row arrays
            dataTable.find('tbody tr').each(function(row) {
                var rowData = (dataTableData[row] = []);
                $(this).children().each(function(col) {
                    rowData[2 * col] = dataTableHeaders[col];
                    rowData[2 * col + 1] = parseFloat($(this).html());
                });
            });
            // return row arrays
            return dataTableData;
        },
        bindOnClick = function() {
            var dataTableData = cacheRows();
            $('#baseTable tbody tr').each(function(row) {
                $(this).on('click', function() {
                    var data = dataTableData[row];
                    logDiv.html(row);
                    alert(data);
                    console.dir(data);
                });
            });
        };
    bindOnClick();
    return {
        refresh: bindOnClick
    };
})();

​ Now, when the tables change, you can preserve the intended behavior by calling tableData.refresh(). This is still a little messy because we never call off, but as we only use jQuery to change the DOM, we rely on it to unbind event handlers if we change elements. It's probably better to change the returned object so it has two functions: one to refresh the cache and one to rebind. rebinding is needed when the whole table was rebuilt and no DOM node is reused. Otherwise, this leaks event handlers. If you update the table infrequently, you should be fine. For frequent updates, I'd change it back to using the index function again.

Edit: Here's an example for what I described in my comment below.

var prepareForPlotting = function(baseSelector, dataSelector, clickhandler) {
    var cacheRows = function() {
            var dataTable = $(dataSelector),
                dataTableHeaders = [],
                dataTableData = [];
            // fetch headers
            dataTable.find('thead th').each(function(col) {
                dataTableHeaders[col] = $(this).html();
            });
            // fetch column data, prepare row arrays
            dataTable.find('tbody tr').each(function(row) {
                var rowData = (dataTableData[row] = []);
                $(this).children().each(function(col) {
                    rowData[2 * col] = dataTableHeaders[col];
                    rowData[2 * col + 1] = parseFloat($(this).html());
                });
            });
            // return row arrays
            return dataTableData;
        },
        bindOnClick = function() {
            var dataTableData = cacheRows();
            $(baseSelector + ' tbody tr').each(function(row) {
                $(this).on('click', function() {
                    clickhandler(row, dataTableData[row]);
                });
            });
        };
    bindOnClick();
    return {
        recache: cacheRows,
        rebind: bindOnClick
    };
}

var logDiv = $('#log'),
    baseHandler = prepareForPlotting('#baseTable', '#dataTable',
        function(row, data) {
            logDiv.html(row);
            alert(data);
            console.dir(data);
        }
    );
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ wow :) Thanks for that. I'm still trying to learn jQuery and I would like to learn the best practices at start, so any advice's like Your are very helpful. Any future updates on this are welcome :) \$\endgroup\$ – Misiu Aug 6 '12 at 6:05
  • 1
    \$\begingroup\$ Your answer is awesome! I'am looking for a way to change this in some kind of plugin, so that I will be able to use it more than ones on page. In my solution I'm using 4 tables like shown here jsfiddle.net/Misiu/9j5Xy/12 this is simple. These 2 solution are used in asp 2.0 webpage, so every time I request data from server (my update panel updates) I must init all javascript again. So can Your awesome code be changed into plogin? So that I can call it like $('wrapper').plotData()? Or should I leave Your solution? Which one is better? \$\endgroup\$ – Misiu Aug 7 '12 at 6:39
  • 1
    \$\begingroup\$ In that case, I'd make it a function with the selectors as parameters. You call it once per table combination and use the return value for refreshes on new data (so you can register it as an eventhandler for a 'change' event you fire when data changes on a table). But I'd first drop the alert and the call to logDiv :-) \$\endgroup\$ – Arne Aug 7 '12 at 7:47
  • 1
    \$\begingroup\$ not the div, I'd pass the selectors for baseTable and dataTable and a callback function. The callback updates the plot (so you can target different plotting areas). \$\endgroup\$ – Arne Aug 7 '12 at 8:02
  • 1
    \$\begingroup\$ I would pass the selectors instead of the div so you can delete the dom nodes and rebuild the table and have it still work. The callback function... I'll add an example in my answer, it would be too long for a comment. \$\endgroup\$ – Arne Aug 7 '12 at 8:29

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.