patternjavascriptMinor
What improvements can I make to this table filtering jQuery Plugin?
Viewed 0 times
thiscanwhatmakepluginjqueryimprovementstablefiltering
Problem
This plugin is meant to filter through a table based on column header text that matches options given to it.
check it out here: Filter Table Plugin
Plugin code below for reference:
```
//framework from http://jqueryboilerplate.com/
; (function ($, window, document, undefined) {
var pluginName = "filterTable",
defaults = {
};
function Plugin(element, options) {
this.element = element;
this.options = $.extend({}, defaults, options);
this._defaults = defaults;
this._name = pluginName;
this.init();
}
Plugin.prototype = {
init: function () {
$el = $(this.element);
$table = $('#' + $el.data('filtertable'));
var filterColumnNames = ($el.attr('data-filtercolumns')).split(',');
var filterColumns = new Array();
var altRowCSS = $el.data('filteralternatingcss');
//get the column index of the column we'll be filtering on
$table.find('th').each(function (i) {
for (j in filterColumnNames) {
if ($(this).text().trim() == filterColumnNames[j]) {
filterColumns.push(i + 1);
}
}
});
$el.keyup(function () {
var query = $(this).val();
var numDisplayed = 0;
//remove any no result warnings
$table.find('#noResultsRow').remove();
//hide all rows
$table.find('tr').removeClass(altRowCSS).hide();
//show the header row
$table.find('tr:first').show();
for (i in filterColumns) {
$table.find('tr td:nth-child(' + filterColumns[i] + ')').each(function () {
if ($(this).text().toLowerCase().indexOf(query.toLowerCase()) >= 0) {
$(this).parent().show();
numDisplayed++;
check it out here: Filter Table Plugin
Plugin code below for reference:
```
//framework from http://jqueryboilerplate.com/
; (function ($, window, document, undefined) {
var pluginName = "filterTable",
defaults = {
};
function Plugin(element, options) {
this.element = element;
this.options = $.extend({}, defaults, options);
this._defaults = defaults;
this._name = pluginName;
this.init();
}
Plugin.prototype = {
init: function () {
$el = $(this.element);
$table = $('#' + $el.data('filtertable'));
var filterColumnNames = ($el.attr('data-filtercolumns')).split(',');
var filterColumns = new Array();
var altRowCSS = $el.data('filteralternatingcss');
//get the column index of the column we'll be filtering on
$table.find('th').each(function (i) {
for (j in filterColumnNames) {
if ($(this).text().trim() == filterColumnNames[j]) {
filterColumns.push(i + 1);
}
}
});
$el.keyup(function () {
var query = $(this).val();
var numDisplayed = 0;
//remove any no result warnings
$table.find('#noResultsRow').remove();
//hide all rows
$table.find('tr').removeClass(altRowCSS).hide();
//show the header row
$table.find('tr:first').show();
for (i in filterColumns) {
$table.find('tr td:nth-child(' + filterColumns[i] + ')').each(function () {
if ($(this).text().toLowerCase().indexOf(query.toLowerCase()) >= 0) {
$(this).parent().show();
numDisplayed++;
Solution
-
You have:
This is redundant as the declaration :
Can be accessed anywhere in your plugin.
-
Declare all variables that you use. This is a really good practice to have change lines like:
to:
JavaScript will let you use them without but you'll run into issues otherwise.
-
Variables need meaningful names so their lives don't seem meaningless and they despair.
Invective! Verb your expletive nouns!
What is
What is
You have:
this._defaults = defaults;This is redundant as the declaration :
var pluginName = "filterTable",
defaults = {
};Can be accessed anywhere in your plugin.
-
Declare all variables that you use. This is a really good practice to have change lines like:
$el = $(this.element);
$table = $('#' + $el.data('filtertable'));to:
var $el = $(this.element);
var $table = $('#' + $el.data('filtertable'));JavaScript will let you use them without but you'll run into issues otherwise.
-
Variables need meaningful names so their lives don't seem meaningless and they despair.
Invective! Verb your expletive nouns!
$table.find('th').each(function (i)
{
for (j in filterColumnNames)
{
if ($(this).text().trim() == filterColumnNames[j])
{
filterColumns.push(i + 1);
}
}
});What is
i? index? columnIndex?What is
j? currentColumnName?Code Snippets
this._defaults = defaults;var pluginName = "filterTable",
defaults = {
};$el = $(this.element);
$table = $('#' + $el.data('filtertable'));var $el = $(this.element);
var $table = $('#' + $el.data('filtertable'));$table.find('th').each(function (i)
{
for (j in filterColumnNames)
{
if ($(this).text().trim() == filterColumnNames[j])
{
filterColumns.push(i + 1);
}
}
});Context
StackExchange Code Review Q#24728, answer score: 2
Revisions (0)
No revisions yet.