2
\$\begingroup\$

I have written a JavaScript function that takes JSON encoded data and then, with conditionals, formats that data into a table.

I asked this question not so long ago but that was just a snippet of this:

Better way to code these deeply nested / multiple if/else statements

This is the full thing with all the structure and bells and whistles. The code is heavily commented, so it should make sense. I feel the structure of the code is easy to read and as it should be. Don't hold back on the criticism! 

 $.getJSON("People.json",
                function(data) {
                    content = '<tr>';
                    content += '<tbody>';
                    for (var i = 0; i < data.length; i++) {
                        var Person = data[i];
                        var swipeStatus;
                        var HolidayType;
                        console.log(Person.IsNightShift);
                        /////////////////Placing the OP Colum no formatting needed//////////////
                        content += '<td>' + Person.Op + ' </td>';

                        // If statment to figue out which kind of holiday they are on if any ///      
                        if (Person.Shift == 281) {
                            HolidayType = 'OnHoliday';
                        } else if (Person.Shift == 282) {
                            HolidayType = 'HolidayAM';
                        } else if (Person.Shift == 283) {
                            HolidayType = 'HolidayPM';
                        } else {
                            HolidayType = null;
                        }

                        /////////////////The main logic for the Name colum//////////////////////
                        //Is the person on holiday?
                        if (HolidayType !== null) {
                            swipeStatus = "OnHoliday";
                            //Nope were they night shift and gone home?
                        } else if (Person.IsNightShift == true && Person.ON !== false) {
                            swipeStatus = "NightHome";
                            //Nope Are they day shift and gone home?
                        } else if (Person.OUT !== false) {
                            swipeStatus = "DayHome";
                            //Nope are they day-afternnon shift and gone home?
                        } else if (Person.DifHours > 1 && Person.ON === false) {
                            swipeStatus = "DayHome";
                            //Nope Have they been on dinner and come back? if so set to Lime
                        } else if (Person.ON !== false) {
                            swipeStatus = "backFromDinner";
                            //Nope are they still on dinner? if so set to Yellow
                        } else if (Person.OFF !== false) {
                            swipeStatus = "outForDinner";
                            //nope Did they clock in? if so set to green
                        } else if (Person.Clock !== false) {
                            swipeStatus = "inNormally";
                            // They never clocked in, Set to red!
                        } else {
                            swipeStatus = "defaultStatus";
                        }
                        //Display the name colum withthe correct formatting
                        content += '<td class="' + swipeStatus + '">' + Person.Name + '</td>';

                        //This is the Logic for the Work hours start and end times colum mainily used to say what type of holiday 
                        //If the person is no on holiday then just display the infomation in plain text
                        if (Person.Clock !== false) {
                            swipeStatus = "Blank";
                            content += '<td class="' + swipeStatus + '">' + Person.WorkHours + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + Person.Start + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + Person.End + ' </td>';
                            // IF the person is on a holiday then set the colum to purple and tell the user they are on holiday
                        } else if (Person.Shift == 281) {
                            swipeStatus = "OnHoliday";
                            content += '<td class="' + swipeStatus + '">' + "Holiday" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Holiday" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Holiday" + ' </td>';
                            // IF the person is on a holiday then set the colum to purple and tell the user they are on holiday
                        } else if (Person.Shift == 282) {
                            swipeStatus = "OnHolidayAM";
                            content += '<td class="' + swipeStatus + '">' + "Hol AM" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Hol AM" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Hol AM" + ' </td>';
                            // IF the person is on a holiday then set the colum to purple and tell the user they are on holiday
                        } else if (Person.Shift == 283) {
                            swipeStatus = "OnHolidayPM";
                            content += '<td class="' + swipeStatus + '">' + "Hol PM" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Hol PM" + ' </td>';
                            content += '<td class="' + swipeStatus + '">' + "Hol PM" + ' </td>';
                            // IF the person is on a holiday then set the colum to purple and tell the user they are on holiday
                        } else {
                            swipeStatus = "Blank";
                            content += '<td -webkit-column-width: 100 px;>' + Person.WorkHours + ' </td>';
                            content += '<td>' + Person.Start + ' </td>';
                            content += '<td>' + Person.End + ' </td>';
                        }


                        ///////////////This is the main logic for the Clockin colum///////////////
                        // if the person has not clocked in set the visability to invisable
                        if (Person.Clock === false) {
                            swipeStatus = "NoClock";
                            // if the person is late by 10 mins set to cyan
                        } else if (Person.Late > 10) {
                            swipeStatus = "ClockEarly";
                            // if the person is early by 10 mins set to orange
                        } else if (Person.Late < -10) {
                            swipeStatus = "ClockLate";
                            //if they are ontime with +-10 mins set to lime 
                        } else {
                            swipeStatus = "ClockOntime";
                        }
                        //put the infomation in to colum with the formatting 
                        content += '<td class="' + swipeStatus + '">' + Person.Clock + '</td>';


                        ///////////////This is the main logic for the OFF colum///////////////
                        //if the person has not swiped off set cell to hidden
                        if (Person.OFF === false) {
                            swipeStatus = "NoClock";
                            //else set to a misleading rule name (no css styling plain cell)
                        } else {
                            swipeStatus = "Blank";
                        }
                        content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';


                        ///////////////This is the main logic for the ON colum same as above with different info///////////////
                        if (Person.ON === false) {
                            swipeStatus = "NoClock";
                            // else set the cell to the same unstyled cell
                        } else {
                            swipeStatus = "Blank";
                        }
                        content += '<td class="' + swipeStatus + '">' + Person.ON + '</td>';


                        // this if statment hides the pertentially undeeded could just put another logic block at the bottom to black every cell in each row if there is 
                        //no clock info insted of blanking each cell of the logic blocks 
                        if (Person.Clock === false) {
                            swipeStatus = "NoClock";
                            content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';
                            //If the person goes home with only have 2 out of the standard 4 swiped through the day and there second swipse is 10 mins less than there assgined home time set to Cyan
                        } else if (Person.DifHours > 1 && Person.ON === false && Person.HomeEarly > 10) {
                            swipeStatus = "ClockEarly";
                            content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';
                            //same as above but 10 mins or over home time
                        } else if (Person.DifHours > 1 && Person.ON === false && Person.HomeEarly < -10) {
                            swipeStatus = "ClockLate";
                            content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';
                            //same as above but with the 10 mins either side
                        } else if (Person.DifHours > 1 && Person.ON === false) {
                            swipeStatus = "ClockOntime";
                            content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';
                            //If the person is night shift and has clocked off at there off time with 10 mins either side set to green
                        } else if (Person.IsNightShift == true && Person.ON !== false && Person.HomeEarlyNight > -10 && Person.HomeEarlyNight < 10) {
                            swipeStatus = "ClockOntime";
                            content += '<td class="' + swipeStatus + '">' + Person.ON + '</td>';
                            //If the person is night shift and has clocked off at there off time 10 mins early
                        } else if (Person.IsNightShift == true && Person.ON !== false && Person.HomeEarlyNight > 10 && Person.HomeEarlyNight < 1000) {
                            swipeStatus = "ClockEarly";
                            content += '<td class="' + swipeStatus + '">' + Person.ON + '</td>';
                            //If the person is night shift and has clocked off at there off time 10 mins Late
                        } else if (Person.IsNightShift == true && Person.ON !== false && Person.HomeEarlyNight < -10) {
                            swipeStatus = "ClockLate";
                            content += '<td class="' + swipeStatus + '">' + Person.ON + '</td>';
                        } else if (Person.OUT === false) {
                            swipeStatus = "NoClock";
                            content += '<td class="' + swipeStatus + '">' + '</td>';
                            //if the person doesn't fit the critria above then they must have 4 swipes and a Person.Home time which is a value that shows how many mins thay are over or under there
                            //offical home times they are, the formatting is alittle stange but if it's > than 10 they have gone 10 miniets or over early
                        } else if (Person.Home > 10) {
                            swipeStatus = "ClockEarly";
                            content += '<td class="' + swipeStatus + '">' + Person.OUT + '</td>';
                            //same as above but gone late
                        } else if (Person.Home < -10) {
                            swipeStatus = "ClockLate";
                            content += '<td class="' + swipeStatus + '">' + Person.OUT + '</td>';
                            //if non of that is true they have been good people and gone when they should!
                        } else {
                            swipeStatus = "ClockOntime";
                            content += '<td class="' + swipeStatus + '">' + Person.OUT + '</td>';
                        }

                        content += '</tr>';
                    }
                    ;
                    content += '</tbody>';
                    content += '<p>';
                    $(content).appendTo("tbody");
                });
\$\endgroup\$
2
  • \$\begingroup\$ One quick remark: The HTML structure you are generating is completely broken: You open a single tr (which instead should be inside the loop, one for each Person), you add a tbody inside of it, which is not allowed, and finally at the end you append everything inside every single already existing tbody. \$\endgroup\$
    – RoToRa
    Commented Aug 28, 2014 at 14:21
  • \$\begingroup\$ And there is an extra p tag that is not allowed inside a table/tbody. \$\endgroup\$
    – RoToRa
    Commented Aug 28, 2014 at 14:22

1 Answer 1

Reset to default
2
\$\begingroup\$

Generally concatenating strings is very slow and memory hogging. Use an array and join it together at the end. E.g.:

var content = [];
content.push("<td>");
content.push(Person.Op);
content.push("</ td>");
return content.join("");

You assign HolidayType depending on Person.Shift but never use it, other than checking for null, but query Person.Shift again the same way later.

Other than that you could set HolidayType using an hash map instead:

var SHIFTS = {
  281: 'OnHoliday',
  282: 'HolidayAM',
  283: 'HolidayPM'
}

var holidayType = SHIFTS[Person.Shift] || "";

Why is the variable called swipeStatus? What does "swipe" mean in this context?

Use one string instead of unnecessarily concatenating here.

content += '<td class="' + swipeStatus + '">' + "Holiday" + ' </td>';

Generally helper function for the table cells would be useful. Something like:

function dataCell(value, class) {
   var dataCell = [];

   dataCell.push("<td");
   if (class) {
     dataCell.push(" class='" + class + "'");
   }
   dataCell.push(">");
   dataCell.push(value);
   dataCell.push("</td>");

   return dataCell.join("");
}

<td -webkit-column-width: 100 px;> => <td style="-webkit-column-width: 100 px;">

Moving it to the style sheet would be even better.

Finally:

  • Consider using a JavaScript template engine instead if building the HTML yourself.
  • Move more/most/all logic to the server.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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