7
\$\begingroup\$

I decided to dive into the world of JS/JQuery by trying to build a simple web app to display all of my various meet results in an easy-to-view manner. For this stage of the review, all it does is generate a table of all the meets and log some stuff to the console when you click on it.

The app gets its data as JSON in a form like:

[{"Date": "MM/DD/YYYY", "Name": "Some Event", "Sport": "Run", "Events": [...]},
 {"Date": "MM/DD/YYYY", "Name": "Some Other Event", "Sport": "Swim", "Events": [...]},
 ...
 ]

and displays them in a table like:

enter image description here

Additionally, I'm storing the "Events" part in each cell so that when I click on a cell, I can eventually do something interesting with the data.

Is there anything I'm doing that violates some key language principles? Is what I'm doing with events too hacky? Is there a simpler way to build up the table? All comments welcome.

<html><head>
<script type="text/javascript" src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js"></script>
<script type="text/javascript">
$(document).ready(function() {
    $.ajax({url: 'http://localhost:8080/all_events',
            cache: false,
            type: 'GET',
            dataType: 'jsonp',
            crossDomain: true,
            success: function(data) {
                var tbl_body = document.createElement("tbody");
                $.each(data, function(idx, elem) {
                    var tbl_row = tbl_body.insertRow();
                    tbl_row.className = idx%2 == 0 ? "even" : "odd";

                    tbl_row.insertCell().
                        appendChild(document.createTextNode(elem.Date));

                    event = document.createTextNode(elem.Name);                 
                    empty = document.createTextNode('');

                    if (elem.Sport == 'Run') {
                        var cell = tbl_row.insertCell();
                        cell.className = 'Event';
                        cell.events = elem.Events;
                        cell.appendChild(event);
                        tbl_row.insertCell().appendChild(empty);
                    }
                    else {
                        tbl_row.insertCell().appendChild(empty);
                        var cell = tbl_row.insertCell();
                        cell.className = 'Event';
                        cell.events = elem.Events;
                        cell.appendChild(event);
                    }
                })
                $("#my_events").append(tbl_body);

                $("td.Event").mouseenter(function() {
                    $(this).css('background-color', 'yellow')
                }).mouseleave(function(){
                    $(this).css('background-color', '')
                }).click(function(ev){
                    $.each(ev.toElement.events, function(idx, elem){
                        console.log(idx, elem);
                    });
                });
            }
    });

    $("#Run,#Swim").mouseenter(function() {
        $(this).css('background-color', 'lime')
    }).mouseleave(function() {
        $(this).css('background-color', '')
    }).click(function(){
        console.log($(this).html())
    });
});

</script>
<style>
table, th, td {
    border: 1px solid black;
}
td {
    text-align: center;
    vertical-align: middle;
    padding: 5px;
}
.even { background: #CCC; }
.odd { background: #FFF; }
</style>
</head>
<body>
<table id='my_events'>
    <thead>
        <tr><th>Date</th><th id='Run'>Run</th><th id='Swim'>Swim</th></tr>
    </thead>
</table>
</body>
</html>
\$\endgroup\$
5
\$\begingroup\$

High Level Notes

My most substantial suggestion is to avoid mixing data logic and view code. Transform your data, as pure data, into the form you would like it to be so that your view code can treat it uniformly. Then your view will be more like a "dumb" mail-merge template, even if you decide to keep using jquery to build it up programmatically. A rewrite should clarify what I mean:

This will take each event that the ajax returns and transform it into data for both of the row cells:

function eventToRowData(event) {
    var nullCell = {"Name": '', "Sport":null, "Events": null},
        isRun = event.Sport == 'Run';
    return {
        "Date": event.Date,
        "RunCell": isRun ? event : nullCell,
        "SwimCell": isRun ? nullCell : event
    };
}

Now all the if...else logic vanishes from your view code:

function buildTable(data) {
    var rowData = data.map(eventToRowData),  
        tableBody = document.createElement("tbody");

    rowData.forEach(makeRow);
    $("#my_events").append(tableBody);

    function makeRow(rowDatum) {
        var row = tableBody.insertRow();
        row.insertCell().appendChild(document.createTextNode(rowDatum.Date));
        addEventCell(rowDatum.RunCell);
        addEventCell(rowDatum.SwimCell);

        function addEventCell(cellData) {
            var cell = row.insertCell();
            cell.className = cellData.Sport ? 'Event' : '';
            cell.Events = cellData.Events;
            cell.appendChild(document.createTextNode(cellData.Name));
        }    
    }
}

See below for full working snippet.

Other Notes

  • To mitigate nesting, avoid defining things like success callbacks inline, unless they are so short they can fit on a single line. This will make both the callback itself and whatever is calling it (in your case, an ajax options object) more readable.
  • Table row striping and hover highlighting belong in css. As a rule, most view things that can be done in css should.
  • Consider ditching jquery. The only good reason to have it in this code is simplifying your ajax call. You could use zepto instead, or do it raw, and save some KB. See this link for more.
  • Consider using a light template library to clean up the view code which builds the table.
  • Consider using mithril.js (an extremely light library with the same philosophy as React) if this becomes more complex, or if you are interested in taking the separation of data view even further.
  • I didn't fix all the trivial things like naming conventions. js is camelCase, css is dash-case by convention.
  • Finally, this may not be relevant, since I get the sense you are just doing this to learn, but unless you have more complex, interactive features, I think server side code rendering a static site is more appropriate for this use case.

var sampleData = [{
    "Date": "MM/DD/YYYY",
    "Name": "Some Event",
    "Sport": "Run",
    "Events": [1, 2]
}, {
    "Date": "MM/DD/YYYY",
    "Name": "Some Other Event",
    "Sport": "Swim",
    "Events": [1, 2]
}, {
    "Date": "MM/DD/YYYY",
    "Name": "Another Event",
    "Sport": "Swim",
    "Events": [1, 2]
}, ]

// Rather than work with data in an inconvenient format, change
// it to a format that suits our view code.
function eventToRowData(event) {
    var nullCell = {"Name": '', "Sport":null, "Events": null},
        isRun = event.Sport == 'Run';
    return {
        "Date": event.Date,
        "RunCell": isRun ? event : nullCell,
        "SwimCell": isRun ? nullCell : event
    };
}

function buildTable(data) {
    var rowData = data.map(eventToRowData),  
    	tableBody = document.createElement("tbody");
    
    rowData.forEach(makeRow);
    $("#my_events").append(tableBody);
    
    function makeRow(rowDatum) {
        var row = tableBody.insertRow();
        row.insertCell().appendChild(document.createTextNode(rowDatum.Date));
        addEventCell(rowDatum.RunCell);
        addEventCell(rowDatum.SwimCell);
        
        function addEventCell(cellData) {
        	var cell = row.insertCell();
        	cell.className = cellData.Sport ? 'Event' : '';
            cell.Events = cellData.Events;
            cell.appendChild(document.createTextNode(cellData.Name));
    	}    
    }
}


$(document).ready(buildTable(sampleData))
table, th, td {
    border: 1px solid black;
}
td {
    text-align: center;
    vertical-align: middle;
    padding: 5px;
}
#my_events tbody tr:nth-child(even) {
   background-color: #ccc;
}
td.Event:hover { background: lime }
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<table id='my_events'>
    <thead>
        <tr>
            <th>Date</th>
            <th id='Run'>Run</th>
            <th id='Swim'>Swim</th>
        </tr>
    </thead>
</table>

\$\endgroup\$
3
\$\begingroup\$

Is there a simpler way to build up the table?

Yes. Consider using template libraries like Mustache or Handlebars. They allow you to write markup with special syntax for interpolation, conditions and iterations. All you have to do is write the markup, and provide the data. Running it through them will produce markup with the data interpolated.

Is there anything I'm doing that violates some key language principles?

Not really. However, there are things we can improve. I believe this is just a matter of being familiar of what CSS and jQuery provides that can make these operations simpler.

$("#Run,#Swim").mouseenter(function() {
    $(this).css('background-color', 'lime')
}).mouseleave(function() {
    $(this).css('background-color', '')
})

$("td.Event").mouseenter(function() {
  $(this).css('background-color', 'yellow')
}).mouseleave(function(){
  $(this).css('background-color', '')
})

This can be done using plain CSS

#Run:hover, #Swim:hover{ background: lime }
td.Event:hover { background: yellow }

I believe the console.log that comes with them is unnecessary. If they're for debugging, you shouldn't forget to remove them.


var tbl_body = document.createElement("tbody");

jQuery allows you to create elements with it

var tblBody = $('<tbody/>');

Additionally, JS uses camelCase by convention, like Java. snake_case is usually Python, or C, or PHP.


var tbl_row = tbl_body.insertRow();
tbl_row.className = idx%2 == 0 ? "even" : "odd";
tbl_row.insertCell().appendChild(document.createTextNode(elem.Date));

jQuery also has DOM operations. Append, adding classes and text are covered by append, addClass and text.

var tableRow = $('<tr/>').addClass(idx%2 ? 'odd' : 'even')
                         .text(elem.Date);
                         .appendTo(tableBody)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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