Cfrtjryk

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 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);

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>`

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!