Merging nested tables into a single table

Question

I made a function that merges nested tables into a single table.

The following function converts the headers. It takes an old table that has nested tables in its rows, takes all the headers of these and creates just one big table with one header.

This is only one part of all the creation for the new table. I basically do the same for the body and the footer.

I was wondering if there is anything I can do to improve my code.

function setTableHeader(oldTableName, newTableName) {
 $('table#'+newTableName).html($('<thead />'));
 var firstRow = $('<tr><th>'+newTableName+'</th></tr>');
 var countryColumn = $('<th rowspan="2">Country</th>');
 var row = $('<tr></tr>');
 var channels = $('<tr></tr>');
 var className = '';
 $('#'+oldTableName+' .budget-inner-table').each(function() {
 var oldInnerTable = $(this);
 var headerText = oldInnerTable.find('h3').eq(0).text();
 className = ((insideText.indexOf('-') > 0) ? /- (.*)/.exec(insideText)[1]: insideText.replace(/ /g, '-')).toLowerCase();
 className = className.replace(' ', '-');
 var column = $('<th></th>').text(insideText).attr('id', className);
 row.append(column);
 // loops through the table row headers
 oldInnerTable.find('.budget-channel-table thead th').each(function() {
 var channelTableHeaders = $(this);
 if (channelTableHeaders.hasClass('month') || channelTableHeaders.hasClass('countryCode')) {
 return;
 }
 else {
 var insideText = channelTableHeaders.text();
 }
 var columnData = $('<th></th>').text(headerText)
 .addClass(className)
 .addClass(headerText.replace(/ /g, '-').toLowerCase().replace('.', ''));
 channels.append(columnData);
 });
 });
 row.prepend(countryColumn);
 // appends all rows to the header
 $('table#'+newTableName+' thead').append(firstRow).append(row).append(channels);
}

• ANeves: I've edited your question to make it clearer. Please take a look at it, and if you disagree with the edit just simply undo it.

Answer 1

Suspicious code

The variable insideText seems to be used before it's defined. Is that a global variable? Avoid global variables as much as possible.

---

This else block seems pointless:

else {
 var insideText = channelTableHeaders.text();
}

---

Something's suspicious here:

$('table#' + newTableName).html($('<thead />'));

Note that ids must be unique in the document. So the table prefix there is redundant.

Readability

For better readability, put spaces around operators, for example instead of this;

var firstRow = $('<tr><th>'+newTableName+'</th></tr>');
$('#'+oldTableName+' .budget-inner-table').each(function() {

Get into the habit to write like this:

var firstRow = $('<tr><th>' + newTableName + '</th></tr>');
$('#' + oldTableName + ' .budget-inner-table').each(function() {

---

The ternary operator can be very useful sometimes. But this expression is too complex and hard to read:

className = ((insideText.indexOf('-') > 0) ? /- (.*)/.exec(insideText)[1]: insideText.replace(/ /g, '-')).toLowerCase();

It would be better to expand this to its equivalent using if-else:

if (insideText.indexOf('-') > 0) {
 className = /- (.*)/.exec(insideText)[1];
} else {
 className = insideText.replace(/ /g, '-').toLowerCase();
}

Helper methods

You are using this kind of code in at least two places:

... = insideText.replace(/ /g, '-').toLowerCase();
... = headerText.replace(/ /g, '-').toLowerCase()

It would be good to introduce a helper function for this, for example:

function slugify(text) {
 return text.replace(/ /g, '-').toLowerCase();
}
... = slugify(insideText);
... = slugify(headerText);

Misc

Code like this can be simplified:

if (condition) {
 return;
} else {
 // do something
}

Like this:

if (condition) {
 return;
}
// do something