Problem
This function below (used on a timeout due to my page loading time) displays information on my site. I grab this data from my table view that I load into the page. It is launched by clicking a button in the table view, and gives that information from the corresponding row. It is very clunky. As you can see in the code, certain values (current year, costume_exists boolean value) change the output of data being displayed. Is there an easier or cleaner way of doing this?
setTimeout(function() {
var table = document.getElementById("bands");
var icon = "</i>";
if (table != null) {
for (var i = 0; i < table.rows.length; i++) {
for (var j = 0; j < table.rows[i].cells.length; j++)
table.rows[i].cells[j].onclick = function() {
if (this.cellIndex == 7) {
if (!this.innerHTML.includes(icon)) {
var $row = $(this).closest("tr");
var $year = $row.find(".year").text();
var $prize = $row.find(".prize").text();
var $band = $row.find(".band").text();
var $mp = $row.find(".mp").text();
var $ge_music = $row.find(".ge_music").text();
var $vp = $row.find(".vp").text();
var $ge_visual = $row.find(".ge_visual").text();
var $costume = $row.find(".costume").text();
var $total = $row.find(".total").text();
var $costumer = $row.find(".costumer").text();
var $designer = $row.find(".designer").text();
var $arranger = $row.find(".arranger").text();
var $choreographer = $row.find(".choreographer").text();
if ($costume.length > 1) {
costume_exists = true;
} else {
costume_exists = false;
}
if ($mp.length > 0) {
playing_exists = true;
} else {
playing_exists = false;
}
breakdown = "breakdown"
if ($year < 1991 && costume_exists) {
breakdown = ('<h3>' + $band + " " + $year + '</h3>' +
'<i>' + getOrdinal($prize) + ' Prize' + '</i><br><br>' +
'<b>Music:</b> ' + $ge_music + '<br>' +
'<b>Presentation:</b> ' + $ge_visual + '<br>' +
'<b>Costume:</b> ' + $costume + '<br><br>' +
'<b>Total Points:</b> ' + $total + '<br><br>' +
'<b>Costumer:</b> ' + $costumer + '<br>' +
'<b>Costume/Set Designer:</b> ' + $designer + '<br>' +
'<b>Music Arranger:</b> ' + $arranger + '<br>' +
'<b>Choreographer:</b> ' + $choreographer + '<br>')
swal({
title: 'Point Breakdown',
html: breakdown
})
} else if (costume_exists) {
breakdown = ('<h3>' + $band + " " + $year + '</h3>' +
'<i>' + getOrdinal($prize) + ' Prize' + '</i><br><br>' +
'<b>Music Playing:</b> ' + $mp + '<br>' +
'<b>General Effect Music:</b> ' + $ge_music + '<br>' +
'<b>Visual Performance:</b> ' + $vp + '<br>' +
'<b>General Effect - Visual:</b> ' + $ge_visual + '<br>' +
'<b>Costume:</b> ' + $costume + '<br><br>' +
'<b>Total Points:</b> ' + $total + '<br><br>' +
'<b>Costumer:</b> ' + $costumer + '<br>' +
'<b>Costume/Set Designer:</b> ' + $designer + '<br>' +
'<b>Music Arranger:</b> ' + $arranger + '<br>' +
'<b>Choreographer:</b> ' + $choreographer + '<br>')
swal({
title: 'Point Breakdown',
html: breakdown
})
} else if (playing_exists) {
breakdown = ('<h3>' + $band + " " + $year + '</h3>' +
'<i>' + getOrdinal($prize) + ' Prize' + '</i><br><br>' +
'<b>Music Playing:</b> ' + $mp + '<br>' +
'<b>General Effect Music:</b> ' + $ge_music + '<br>' +
'<b>Visual Performance:</b> ' + $vp + '<br>' +
'<b>General Effect - Visual:</b> ' + $ge_visual + '<br><br>' +
'<b>Total Points:</b> ' + $total + '<br><br>' +
'<b>Costumer:</b> ' + $costumer + '<br>' +
'<b>Costume/Set Designer:</b> ' + $designer + '<br>' +
'<b>Music Arranger:</b> ' + $arranger + '<br>' +
'<b>Choreographer:</b> ' + $choreographer + '<br>')
swal({
title: 'Point Breakdown',
html: breakdown
})
} else {
alert("No point breakdowns for " + $year + " are available.");
}
}
}
}
}
};
}, 700);
Solution
I don’t have much experience with JavaScript, so I apologize if my suggestions aren’t idiomatic (or runnable) JS. Here are a few things I see.
Consistent spacing
You switch between tabs of two spaces and four spaces. It’s not clear to me why.
Use braces for multi-line for loops
Your second for loop is missing curly braces. It makes things easier to read to include curly braces if the for loop has code that spans multiple lines.
Use the boolean directly
You can assign the boolean value of an expression directly to a variable.
if ($costume.length > 1) {
costume_exists = true;
} else {
costume_exists = false;
}
if ($mp.length > 0) {
playing_exists = true;
} else {
playing_exists = false;
}
Becomes:
costume_exists = $costume.length > 1;
playing_exists = $mp.length > 0;
Use template literals
You can embed expressions into template strings directly. The parenthesis around the expression look to be unnecessary.
breakdown = ('<h3>' + $band + " " + $year + '</h3>' +
'<i>' + getOrdinal($prize) + ' Prize' + '</i><br><br>' +
'<b>Music:</b> ' + $ge_music + '<br>' +
'<b>Presentation:</b> ' + $ge_visual + '<br>' +
'<b>Costume:</b> ' + $costume + '<br><br>' +
'<b>Total Points:</b> ' + $total + '<br><br>' +
'<b>Costumer:</b> ' + $costumer + '<br>' +
'<b>Costume/Set Designer:</b> ' + $designer + '<br>' +
'<b>Music Arranger:</b> ' + $arranger + '<br>' +
'<b>Choreographer:</b> ' + $choreographer + '<br>')
Becomes:
breakdown = `<h3>{$band} {$year}</h3>
<i>{getOrdinal($prize)} Prize</i><br><br>
<b>Music:</b> {$ge_music}<br>
<b>Presentation:</b> {$ge_visual}<br>
<b>Costume:</b> {$costume}<br><br>
<b>Total Points: {$total}<br><br>
<b>Costumer:</b> {$costumer}<br>
<b>Costume/Set Designer:</b> {$designer}<br>
<b>Music Arranger:</b> {$arranger}<br>
<b>Choreographer:</b> {$choreographer}<br>`
Edit: If getOrdinal() is breaking in the template, a hacky way to get around this is define const prizeOrd = getOrdinal($prize); then use {prizeOrd} in its place.
Reduce nesting
You can reduce nesting by inverting and combining your if-statements.
table.rows[i].cells[j].onclick = function() {
if (this.cellIndex == 7) {
if (!this.innerHTML.includes(icon)) {
// code
}
}
}
Becomes:
table.rows[i].cells[j].onclick = function() {
if (this.cellIndex != 7 || this.innerHTML.includes(icon)) {
return;
}
// code
}
More nesting can be reduced, like the table != null check.
Avoid unneeded duplication
A lot of your lines are taken up by constructing separate (but very similar) strings depending on a few conditions. With template literals, we can use one string with inner expressions.
Define a string for the music portion as such:
music = $year < 1991 && costume_exists
? `<b>Music:</b> {$ge_music}<br>`
: `<b>Music Playing:</b> {$mp}<br>`;
I can’t verify if that’s valid syntax for JS, but you get the gist. Likewise with the costume portion.
It’s not the exact same behavior, but this seems to be what you’re going for:
breakdown = "breakdown"
if ($year > 1991 && !costume_exists && !playing_exists) {
alert(`No point breakdowns for {$year} are available.`);
return;
}
music = $year < 1991 && costume_exists
? `<b>Music:</b> {$ge_music}<br>`
: `<b>Music Playing:</b> {$mp}<br>`;
costume = custume_exists
? `<b>Costume:</b> {$costume}<br><br>`
: '';
breakdown = `<h3>{$band} {$year}</h3>
<i>{getOrdinal($prize)} Prize</i><br><br>
{music}
<b>Presentation:</b> {$ge_visual}<br>
{costume}
<b>Total Points: {$total}<br><br>
<b>Costumer:</b> {$costumer}<br>
<b>Costume/Set Designer:</b> {$designer}<br>
<b>Music Arranger:</b> {$arranger}<br>
<b>Choreographer:</b> {$choreographer}<br>`
swal({
title: 'Point Breakdown',
html: breakdown
})
JavaScript after loading
This function below (used on a timeout due to my page loading time) displays information on my site.
Normally, you would do this as such:
window.onload = function ...
This also should work consistently, whereas using a pre-defined timeout duration may not work with extremely slow internet.
Use const
You can use the const keyword instead of var for the icon variable.
const icon = "</i>";
Given that this is JavaScript, it will make zero difference performance-wise. But, if the value isn’t meant to be changed, this gives an indication to those who read your code.
Performance
Currently you iterate over all cells in the table, O(mn)
. I am not familiar enough with JavaScript to suggest something with better performance.
However I would certainly look into this if your tables become large.
Conclusion
Here is the code I ended up with:
window.onload = function() {
const table = document.getElementById("bands");
const icon = "</i>";
if (table != null) {
for (var i = 0; i < table.rows.length; i++) {
for (var j = 0; j < table.rows[i].cells.length; j++) {
table.rows[i].cells[j].onclick = function() {
if (this.cellIndex !== 7 || this.innerHTML.includes(icon)) {
return;
}
const $row = $(this).closest("tr");
const $year = $row.find(".year").text();
const $prize = $row.find(".prize").text();
const $band = $row.find(".band").text();
const $mp = $row.find(".mp").text();
const $ge_music = $row.find(".ge_music").text();
const $vp = $row.find(".vp").text();
const $ge_visual = $row.find(".ge_visual").text();
const $costume = $row.find(".costume").text();
const $total = $row.find(".total").text();
const $costumer = $row.find(".costumer").text();
const $designer = $row.find(".designer").text();
const $arranger = $row.find(".arranger").text();
const $choreographer = $row.find(".choreographer").text();
const costume_exists = $costume.length > 1;
const playing_exists = $mp.length > 0;
var breakdown = "breakdown"
if ($year > 1991 && !costume_exists && !playing_exists) {
alert(`No point breakdowns for {$year} are available.`);
return;
}
const music = $year < 1991 && costume_exists
? `<b>Music:</b> {$ge_music}<br>`
: `<b>Music Playing:</b> {$mp}<br>`;
const costume = custume_exists
? `<b>Costume:</b> {$costume}<br><br>`
: '';
breakdown = `<h3>{$band} {$year}</h3>
<i>{getOrdinal($prize)} Prize</i><br><br>
{music}
<b>Presentation:</b> {$ge_visual}<br>
{costume}
<b>Total Points: {$total}<br><br>
<b>Costumer:</b> $costumer<br>
<b>Costume/Set Designer:</b> {$designer}<br>
<b>Music Arranger:</b> {$arranger}<br>
<b>Choreographer:</b> {$choreographer}<br>`
swal({
title: 'Point Breakdown',
html: breakdown
})
}
}
}
}
};
I have no way to verify that it works, but it looks like it should.
Hope this helps!
As most of the points have been covered in the other answer I will just add some more.
Alert is evil
Do not use alert. There are only some very limited reasons to use them. This is not one of them.
Reasons not to use them.
- They are blocking and stop all Javascript from running while they are up.
- They they prevent the client from navigating until they have been cleared.
- They are annoying and require user interaction (in this case) when none is needed.
- They can not be trusted. Clients can opt to have all alerts disabled so you can never be sure they are displayed.
- They are ugly as sin.
- They are too late. User interaction should never be a guessing game.
For this type of interaction you are far better to predetermine if an item can be clicked and use the cursor to show if it can be clicked. A simple cursor as pointer <style>.can-click { cursor: pointer; }</style> and a tooltip is all that is needed to indicate that the item can be clicked, rather than a page blocking intrusive pointless alert (I never return to a sites that do that).
If you must have a dialog the use a custom dialog.
<dialog id="noBreakdownsEl">
<p>No point breakdowns for <span id="dialogYearEl"></span> are available.</p>
</dialog>
<script>
setTimeout(() => {
dialogYearEl.textContent = 1992;
noBreakdownsEl.showModal();
},1000);
</script>
Keep it D.R.Y. (Don’t Repeat Yourself)
You code is full of repeated code and data. As a programer, each repeated string of source code (especially data) should annoy you. It is very rare that you will need to repeat data, and if you are repeating source code, it should be in a function.
HTML is for transport
HTML is specifically designed for transport (eg from server to client over network) It is not a client side rendering language, you have the DOM API for that
Reasons not to add HTML via JS
- It is slow (very very slow).
- It encourages you to add content into the code. This means that content changes require a full testing release cycle. Not something to take on lightly.
- That only expert coders can make changes to content if markup in code (that’s a HTML jockeys job)
- Adding HTML (‘element.innerHTML=foo’) to a page resets all the event listeners.
- Did I say it was SLOW….
You should use template element so you can store the HTML on the page where it belongs. See rewrite
One handler.
I don’t know how many cells you have on the page, but you add an event handler for each (even the ones that can not be clicked)
You can use a single event listener that accepts a click on the table. You then use the event object passed to the listener to workout what to do with that click.
Granular code is good code.
You have one big function that does everything. This is not good as it make the code harder to read, debug, and most important modify.
Break the code into discrete functions that have a very clear job to do. Use functions to replace repeated sections of similar code.
The general rules of thumb for functions is
- Longer than a page is too long.
- Does more than one thing? Would be better as two functions.
- Don’t go function crazy, creating a one line function used only once is too granular. A function should make the code smaller.
Rewrite
As you call swal that expects markup I do not clone the template and just add the data to the elements. When all data elements are filled I then get the templates innerHTML that is passed to the function swal
However you should clone the template and add it directly to the page. Depending on what swal does of course.
I could not see any need for jQuery and added a helper queryDOM to make DOM API calls a little less verbose.
There is only one event listener on the table and it checks the target element for the added property templateType If the element has that property then it is processed.
I am also assuming that the number of cells are small (less than a few 100). If not then you should scan the cells (in function addBandClick) using a timeout event (a few at a time) so that you do not block the page.
The named items are taken from the template element as a data attribute dataset.itemNames however you could extract those names from the template elements saving some duplication in the markup.
Note this may not work as the click listener will be removed if you add any content as markup.
const queryDOM = (qStr, element = document) => element.querySelector(qStr);
function itemText(row, name) {
const cell = queryDOM("." + name, row);
const text = cell ? cell.textContent : ""
return name === "prize" ? getOrdinal(text) : text;
}
function bandHTML(cell) {
const template = queryDOM("#template" + cell.templateType);
for (const name of template.dataset.itemNames.split(",")) {
const element = queryDOM(`span [data-name="${name}"]`, template);
element.textContent = itemText(cell.closestRow, name);
}
return template.innerHTML;
}
function bandClicked(event) {
if (event.target.templateType) {
swal({title: 'Point Breakdown', html: bandHTML(event.target)});
}
}
function findClickableCells(table) {
for (const row of table.rows) {
for (const cell of row.cells) {
if (cell.cellIndex === 7 && !queryDOM("i",cell)) {
const year = Number(itemText(row, "year"));
const costume = itemText(row, "costume") !== "";
let type;
if (year > 1991 && costume) { type = "A" }
else if (costume) { type = "B" }
else if (itemText(row, "mp") !== "") { type = "C" }
if (type) {
cell.templateType = type; // Must have, to be clickable
cell.classList.add("can-click");
ceil.closestRow = row;
} else {
cell.title = `No point breakdowns for ${year} are available.`;
}
}
}
}
}
const table = queryDOM("#bands");
if (table) {
findClickableCells(table);
table.addEventListener("click", bandClicked);
}
CSS class so that the client has feedback indicating what can be clicked.
.can-click { cursor: pointer; }
HTML template example. You would create one for each display type. eg id="templateA", templateB, templateC
<template id="templateA" data-item-names="year,mp,costume,band,prize,band,ge_music,ge_visual,total,costumer,designer,arranger,choreographer">
<h3><span data-name="band"></span> <span data-name="year"></span></h3>
<i><span data-name="prize" Prize</i><br><br>
<b>Music:</b> <span data-name="ge_music"></span><br>
<b>Presentation:</b> <span data-name="ge_visual"></span><br>
<b>Costume:</b> <span data-name="costume"></span><br><br>
<b>Total Points: <span data-name="total"></span><br><br>
<b>Costumer:</b> <span data-name="costumer"></span><br>
<b>Costume/Set Designer:</b> <span data-name="designer"></span><br>
<b>Music Arranger:</b> <span data-name="arranger"></span><br>
<b>Choreographer:</b> <span data-name="choreographer"></span><br>
</template>