Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have created a MS Excel type of grid in jQuery. I want to learn best practices and I need your comments for more optimized code.

Please review the code and offer your suggestions.

Demo

jQuery:

var JS = JS || {};

JS.training = JS.training || {};
  JS.training.tableData = JS.training.tableData || {};
  JS.training.tableData = {  

    defaults: {
        $table: $('#myTable'),
        addTableRowBtn: $('#addRowBtn'),
        addTableColBtn: $('#addColBtn')
    },
    createTable: function () {
        var _this = this,
            table = "";
        table += "<thead>";
        for (i = 0; i < 5; i++) {
            var tableHeader = (i == 0) ? "<th style='border:1px solid #E5E5E5; background:#F1F1F1'></th>" : "<th style='border:1px solid #E5E5E5; background:#F1F1F1'>A" + i + "</th>";
            table += tableHeader;
        }
        table += "</thead>";
        table += "<tbody>";
        for (i = 0; i < 5; i++) {
            table += "<tr  id='row" + i + "'>";
            for (var j = 0; j < 5; j++) {
                var tableDataCells = (j == 0) ? "<td width='3%' style='border:1px solid #E5E5E5;'>" + (i + 1) + "</td>" : "<td width=100px style='border:1px solid #E5E5E5' id='td" + j + "'  contenteditable=true> </td>";
                table += tableDataCells;
            }
            table += '</tr>';
        }
        table += "</tbody>";
        //APPEND TABLE MARKUP
        $(_this.defaults.$table).append(table);
        //BIND EVENTS
        _this.bindEvents();
    },
    addTableRow: function () {
        var _this = this,
            colLen = $("#myTable tr:nth-child(1) td").length,
            colVal = parseInt($("#myTable tr:last-child td:first").text()) + 1;
        for (i = 0; i < 1; i++) {
            var table = "<tr id=row" + colVal + "'>";
            for (var j = 0; j < colLen; j++) {
                if (j == 0) {
                    table += '<td width="3%" style="border:1px solid #E5E5E5; background:#F1F1F1">' + colVal + ' </td>';
                } else {
                    table += "<td width=100px style='border:1px solid #E5E5E5;' contenteditable=true id='td" + j + "'> </td>";
                }
            }
            table += '</tr>';
        }
        $(_this.defaults.$table).append(table);
    },
    addTableColumn: function () {
        var _this = this,
            colVal = $("#myTable tr th:last-child").text(),
            colNum = parseInt(colVal.charAt(1)) + 1;
        console.log(colNum);
        $("#myTable thead tr:last-child").append("<th  style='border:1px solid #E5E5E5; background:#F1F1F1'>A" + colNum + "</th>");
        $("#myTable tbody tr").each(function () {
            $(this).append("<td width=100px style='border:1px solid #E5E5E5;' contenteditable=true id='td" + colNum + "'></td>")
        });
    },
    bindEvents: function () {
        var _this = this;
        //CAPTURE ADD ROW BUTTON CLICK
        _this.defaults.addTableRowBtn.on('click', function () {
            _this.addTableRow();
        });

        //CAPTURE ADD COLUMN BUTTON CLICK
        _this.defaults.addTableColBtn.on('click', function () {
            _this.addTableColumn();
        });
    },
    init: function () {
        var _this = this;
        _this.createTable();
    }
};

//INIT CALL
JS.training.tableData.init();

HTML:

<div id="wrapper">
    <button id="addRowBtn" name="addRowBtn" value="Add Row">Add Row</button>
    <button id="addColBtn" name="addColBtn" value="Add Col">Add Column</button>
    <div id="table">
        <table id="myTable" cellpadding="0" cellspacing="0" border="0" style="border:1px solid #E5E5E5"></table>
    </div>
</div>

CSS:

body {
    font:normal 14px/16px Arial, Helvetica, sans-serif
}
#wrapper {
    margin:100px auto 0;
    width:80%;
}
#myTable {
    width:100%;
    margin:20px auto 0
}
#myTable th, #myTable td {
    padding:5px;
}
#myTable tr td.first {
    background:#F1F1F1;
    border:1px solid #E5E5E5;
}
share|improve this question

3 Answers 3

From a once over:

  • The column and row count should be a parameter/variable/default
  • The namespace JS.training seems like overkill, I doubt anybody would ever use 2 libraries for showing datagrids ;)
  • The following could use extraction of the pattern, also you should look into a templating library I am using my personal template function here as an example:

        for (i = 0; i < 5; i++) {
            var tableHeader = (i == 0) ? 
            "<th style='border:1px solid #E5E5E5; background:#F1F1F1'></th>" : 
            "<th style='border:1px solid #E5E5E5; background:#F1F1F1'>A" + i + "</th>";
            table += tableHeader;
        }
    

    could be

    function fillTemplate( s )
    { //Replace ~ with further provided arguments
      for( var i = 1, a = s.split('~'), s = '' ; i < arguments.length ; i++ )
        s = s + a.shift() + arguments[i];
      return s + a.join("");
    }     
    
    var headerTemplate = `<th style='border:1px solid #E5E5E5; background:#F1F1F1'>~</th>`    
    for (i = 0; i < 5; i++) {
      var header = (i == 0) ? "" : i;
      table += fillTemplate( headerTemplate, header );
    }
    
  • I would split up the creation of header and items HTML into 2 separate functions

  • var _this = this is really only useful for closures, and you are not using those in createTable.
  • You should extract <td width="3%" style="border:1px solid #E5E5E5; background:#F1F1F1"> into a template as well, you use it both in createTable and addTableRow.
  • Same goes for the header template which you use as well in addTableColumn
  • These comments can be filed under Captain Obvious and removed:

    //CAPTURE ADD ROW BUTTON CLICK
    _this.defaults.addTableRowBtn.on('click', function () {
        _this.addTableRow();
    });
    
    //CAPTURE ADD COLUMN BUTTON CLICK
    _this.defaults.addTableColBtn.on('click', function () {
        _this.addTableColumn();
    });
    

All in all I really like your code, I will leave it to @kleinfreund to review your CSS.

share|improve this answer

Only some quick notes on your HTML/CSS.

HTML:

  1. You're repeating ID's in your generated markup. ID's must only occur once. If you need them in multiple locations, use classes instead.

CSS:

  1. You declare a line-height of 16 pixels. I'd advice you using a unit-less relative value for line-height like this: font: 14px/1.2 Arial, Helvetica, sans-serif;

    Otherwise you would need to redefine the line-height on all elements with a higher font-size, because they currently have a line-height of 16 pixels as well. With a relative value you do this only once.

    I also ommited normal in your declaration, because the default values for font-style, font-weight and font-variation are normal anyway.

  2. Although it's possible to ommit the last ; in a rule declaration, it's not a good practise. You will run into troubles while editing CSS. By always including it, you don't need to worry about issues like moving the declaration further to the top, etc.

  3. You add some CSS rules as inline styles to all your generated td's. You can move these rules to your CSS to reduce this unnecessary repetition.

share|improve this answer

Some further points...

  • You have inline CSS in your tds, this could easily be extracted into your stylesheet.

  • I much prefer creating HTML using jQuery, rather than ugly concatenated strings:

    var $table = $('<tbody>')
    
    for (i = 0; i < 5; i++) {
        var $row = $('<tr>');
        for (var j = 0; j < 5; j++) {
            var $cell = (j == 0)
              ? $('<td>').text(i + 1) 
              : $('<td>').attr('contenteditable', true);
            $cell.appendTo($row);
        }
        $row.appendTo($table);
    }
    
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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