Rendering a person's time clock record in HTML, with many color-coded conditions

Question

I have coded this mess of JavaScript and a bit of jQuery. Everything here works perfect as intended, however it is a real mess. What would be the best way to clean this up with best practices and such?

$.getJSON("People.json",
    function(data) {
        $.each(data.People, function(i, PersonObj) {
            var Person = PersonObj[Object.keys(PersonObj)[0]];

            content = '<tr>';
            content += '<tbody>';
            content += '<td>' + Person.Op + ' </td>';

            // if the person has swiped off but not back on and one hour has elapsed                                  
            if (Person.DifHours > 1 && Person.ON === false) {
                content += '<td style=" background: red;">' + Person.Name;
            } else if (Person.OUT !== false) {
                content += '<td style=" background: red;">' + Person.Name;
                // If the person has swiped back on from dinner set to lime 
            } else if (Person.ON !== false) {
                content += '<td style=" background: lime;">' + Person.Name;
                // If the person has swipped of for dinner but 1 hour has not elapsed set to yellow
            } else if (Person.OFF !== false) {
                content += '<td style=" background: yellow;">' + Person.Name;
            } else if (Person.Clock !== false) {
                // If the person has clocked on set to lime
                content += '<td style=" background: lime;">' + Person.Name;
            } else {
                content += '<td style=" background: Red;">' + Person.Name;
            }
            // putting values in to the table without formatting 

            content += '<td>' + Person.WorkHours + ' </td>';
            content += '<td>' + Person.Start + ' </td>';
            content += '<td>' + Person.End + ' </td>';

            // if the person has not clocked in set the text to white
            if (Person.Clock === false) {
                content += '<td style="  background: white; color: white;">' + Person.Clock;
            }
            // if the person is late by 10 mins set to cyan
            else if (Person.Late > 10) {
                content += '<td style=" background: cyan;">' + Person.Clock;
                // if the person is early by 10 mins set to orange
            } else if (Person.Late < -10) {
                content += '<td style=" background: Orange;">' + Person.Clock;
                // Setting text color based on conditions 
            } else {
                content += '<td style=" background: Lime;">' + Person.Clock + ' </td>';
            }
            // Setting text color based on conditions 
            if (Person.OFF === false) {
                content += '<td style=" background: white; color: white;">' + Person.OFF;
            } else {
                content += '<td>' + Person.OFF + ' </td>';

            }// Setting text color based on conditions 
            if (Person.ON === false) {
                content += '<td style=" background: white; color: white;">' + Person.ON;
            } else {
                content += '<td>' + Person.ON + ' </td>';

            }


             if (Person.Clock == false){
                 content += '<td style=" background: white; color: white;">' + Person.OUT;
            }else if (Person.DifHours > 1 && Person.ON == false && Person.HomeEarly > 10){
            content += '<td style=" background: cyan;">' + Person.OFF;
            }else if (Person.DifHours > 1 && Person.ON == false && Person.HomeEarly < - 10){
                content += '<td style=" background: orange;">' + Person.OFF;
            }else if(Person.DifHours > 1 && Person.ON == false){
                content += '<td style=" background: lime;">' + Person.OFF;
            }else if (Person.OUT === false) {
                content += '<td style=" background: white; color: white;">' + Person.OUT;
            }
            //Setting home colum formatting based on if they are in or out and if they locked early or late
            else if (Person.Home > 10) {
                content += '<td style=" background: cyan;">' + Person.OUT;
            } else if (Person.Home < -10) {
                content += '<td style=" background: Orange;">' + Person.OUT;
            }else {
                content += '<td style=" background: Lime;">' + Person.OUT + ' </td>';
            }

            content += '</tr>';
            content += '</tbody>';
            content += '<p>';
            $(content).appendTo("tbody");

        });
    });

Answer 1

There are a few good practices that will help this code out.

Close the HTML tags that you open - generate valid HTML

Newer browsers will probably render your content properly, but older browsers may not. You may also get strange formatting errors that are hard to debug. Save yourself the headache and always close your tags in the reverse order that you open them.

<tbody><tr><td>....</td></tr></tbody>

You may also want to put the <tbody> tags outside of your foreach loop, as there should be one <tbody> for the overall table, not one per row.

You can also use the W3C official HTML validator to make sure that the output you generate is valid HTML, and adjust your JavaScript to match.

Set a variable in the if block; construct the content only once

This makes it easier to update if you later decide to change how the raw HTML is built (like moving from tables to divs and spans).

Avoid CSS shorthands if you are not using all of the properties

Using background: XYZcolor; means that any other CSS background properties (background-image, background-repeat, background-attachment, background-position) already set are being wiped out. If this is your intention, keep it. If not, be more precise with your styles and use only background-color: XYZcolor;. This may not affect you now, but is a good habit to keep to avoid surprises later.

Use CSS classes; avoid using the style= attribute

This allows you to:

1. Give meaningful names indicating why the cell is colored a particular way (so you can debug using "view source" in the browser)
2. Adjust the styles independently of your JavaScript (so that you don't need to involve a JavaScript developer if the client wants the red cells to be just a bit redder)
3. Avoid the problems mentioned in this SO question: What's so bad about in-line CSS?

Combining with the earlier suggestions gives us:

var swipeStatus;
 // if the person has swiped off but not back on and one hour has elapsed  
if (Person.DifHours > 1 && Person.ON === false) {
    swipeStatus = "lateByOneHour";
} else if (Person.OUT !== false) {
    swipeStatus = "notOutYet";
    // If the person has swiped back on from dinner set to lime 
} else if (Person.ON !== false) {
    swipeStatus = "backFromDinner";
    // If the person has swiped of for dinner but 1 hour has not elapsed set to yellow
} else if (Person.OFF !== false) {
    swipeStatus = "outForDinner";
} else if (Person.Clock !== false) {
    // If the person has clocked on set to lime
    swipeStatus = "inNormally";
} else {
    swipeStatus = "defaultStatus";
}
content += '<td class="'+ swipeStatus +'">' + Person.Name + '</td>';

With the appropriate CSS:

.lateByOneHour {
    background-color: red;
}
.notOutYet {
    background-color: red;
}
.backFromDinner {
    background-color: lime;
}
.outForDinner {
    background-color: yellow;
}
.inNormally {
    background-color: lime;
}
.defaultStatus {
    background-color: red;
}

A similar strategy can be applied to the other logic blocks of your code.

You can also add multiple CSS classes to a single tag <td class="class1 class2">. Sometimes that is useful in simplifying if/else logic, but doesn't appear to be the case here. Good to keep in your back pocket though.

For better performance, call appendTo() outside of the loop

The jQuery site itself has a page about why append() is slow in loops, but the gist of it is that it is expensive to touch the DOM, as append() and appendTo() do. Appending a giant chunk of HTML (your whole table) once at the end of the loop will perform faster than appending little bits of HTML (just a row) in each iteration of the loop.

Answer 2

That's a big mess of conditionals. Hopefully they all match the comments near them, though given typical code that's likely not the case. You are repeating a lot of HTML code that could be addressed using variables as @V31 demonstrated.

If this is the entirety of the code I would probably stop there, but as this system grows you may want to generalize the conditionals and how they apply. First, you can create short functions to help ensure the comments match the conditionals and are clear to future maintainers, reduce the code duplication (DRY), and possibly allow reuse in other areas.

The very first conditional provides a good example.

// if the person has swiped off but not back on and one hour has elapsed                                  
if (Person.DifHours > 1 && Person.ON === false) {

I see no reference to Person.OFF. Is this a bug or intentional? Does the system ensure that Person.ON === false forces Person.OFF to be true? Assuming the code is correct we can define

function hasBeenOutForLongerThanOneHour(person) {
    return !person.ON && person.DifHours > 1;
}

Applying this to the rest of the conditions in the first set yields

function isOut(person) {
    return person.OUT;
}
function isBackFromDinner(person) {
    return person.ON;
}
function hasBeenOnBreakLessThanOneHour(person) {
    return person.OFF;
}
function hasClockedOn(person) {
    return person.Clock;
}

I used your comments for the function names, but as you can probably see some are more descriptive than others and some appear to be underspecified. For example, isBackFromDinner looks only at Person.ON, but won't someone who just arrived at work also be on? You wouldn't call this person "back from dinner" if they haven't gone to dinner yet. And calling it a "break" is more-widely applicable than "dinner".

Armed with these functions, you can now replace those conditional chains with data tables. Each table will map the functions to the style and value to use for building the cell.

• All of the styles boil down to two variations: hide the cell with white-on-white or apply a background color. Let's indicate the first option using the color hide.

• The cell value itself varies in the 2nd and 3rd blocks, but let's add it to every table for consistency.

Here's the table that drives the Name cell:

var NAME_STYLE_FILTERS = [
        // filter function, field name, style to apply
        [hasBeenOutForLongerThanOneHour, 'Name', 'red'],
        [isOut, 'Name', 'red'],
        [isBackFromDinner, 'Name', 'lime'],
        [hasBeenOnBreakLessThanOneHour, 'Name', 'yellow'],
        [hasClockedOn, 'Name', 'lime'],
        [null, 'Name', 'red'],
    ];

Note: I chose arrays since there are only three elements each, but using objects may make it clearer what's going on.

Since you'll be using this table format with each conditional block, we'll create a function to build the table cell using the data tables and person.

function buildStyledCell(person, filters) {
    var match,
        text,
        style;
    filters.some(function (filter) {
        if (!style[0] || style[0](person) {
            match = filter;
            return true;
        }
    };
    text = person[match[1]];
    style = match[2] == 'hide' ? 'background-color: white; color: white'
            : 'background-color: ' + match[2];
    return '<td style="' + style + '">' + text + '</td>';
}

Put it all together and you have this:

var content = '<tr>'
content += '<tbody>';
content += '<td>' + Person.Op + '</td>';
content += buildStyledCell(Person, NAME_STYLE_FILTERS);
content += '<td>' + Person.WorkHours + ' </td>';
content += '<td>' + Person.Start + ' </td>';
content += '<td>' + Person.End + ' </td>';
content += buildStyledCell(Person, CLOCK_STYLE_FILTERS);
content += buildStyledCell(Person, OFF_OUT_STYLE_FILTERS);
content += '</tr>';
content += '</tbody>';
content += '<p>';
$(content).appendTo("tbody");

BTW, what's with your tags here? This doesn't look valid:

<tr>
    <tbody>             <-- why? only one per table
        <td>...</td>
        ...
    </tr>
</tbody>                <-- out of order with </tr>, but again not needed

General Advice

• As long as you're using Object.keys you may as well use Array.prototype.forEach which is also from ES5. Be aware that it will fail if data.People is null or undefined--just like $.each.

  data.People.forEach(function (PersonObj) { ... });

• When I read

  if (Person.OUT !== false)
  

  I immediately ask myself, "What other values besides true and false can Person.OUT take on, and what do they mean? Does null mean we don't know if they are out, or will it be undefined if they haven't clocked in yet today?"

  It's more likely that ON, OFF, and OUT will always contain boolean values, and using === and !== only complicates the code. That you use Person.ON == false later on exacerbates the reader's confusion.

  Simplify these to

  if (Person.OUT)
  

  and

  if (... && !Person.ON && ...)
  

  if you can, which read more like prose than code.

• You're not closing the <td> elements inside the conditionals.

• You can drop this super-helpful comment that appears several times:

  // Setting text color based on conditions
  

Answer 3

The first if block can be narrowed by using a color variable; something like this:

var color = "red";
// if the person has swiped off but not back on and one hour has elapsed                                  
if (Person.DifHours > 1 && (Person.ON === false || Person.OUT !==false)) {
     color = "red";
 // If the person has swiped back on from dinner set to lime 
 } else if (Person.ON !== false || Person.Clock !== false) {
     color = "lime";
 // If the person has swiped off for dinner but 1 hour has not elapsed set to yellow
 } else if (Person.OFF !== false) {
     color = "yellow";
 } 
 content += "<td style=\" background: "+color+";\">" + Person.OUT;

You can do the same thing for the rest of your if blocks. If you combine conditions that result in the same color with && and ||, you can reduce code length while making it more readable.

Answer 4

As usual, I'm a day late and a dollar short. :)

You actually have a good case for using a view model. All of the ifs would be methods in your view model to return the proper CSS class for your <td> tags. This has a couple benefits:

1. The view logic is now its own component and can be tested using any of the JavaScript unit testing frameworks out there
2. The code for concatenating the HTML becomes much leaner and cleaner.

Furthermore, you can get a slight speed boost by appending strings to an Array instead of continually doing string concatenation.

EmployeeStatusViewModel.js

function EmployeeStatusViewModel(person) {
    this.person = person || null;
}

EmployeeStatusViewModel.prototype = {

    person: null,

    constructor: EmployeeStatusViewModel,

    getOffStatus: function() {
        return this.person.OFF ? "out" : "";
    },

    getOnStatus: function() {
        return this.person.ON ? "on" : "";
    },

    getSwipeStatus: function() {
        var swipeStatus = "defaultStatus";

        // if the person has swiped off but not back on and one hour has elapsed  
        if (this.person.DifHours > 1 && !this.person.ON) {
            swipeStatus = "lateByOneHour";
        } else if (this.person.OUT) {
            swipeStatus = "notOutYet";
        // If the this.person has swiped back on from dinner set to lime 
        } else if (this.person.ON) {
            swipeStatus = "backFromDinner";
        // If the this.person has swiped of for dinner but 1 hour has not elapsed set to yellow
        } else if (this.person.OFF) {
            swipeStatus = "outForDinner";
        // If the person has clocked on set to lime
        } else if (this.person.Clock) {
            swipeStatus = "inNormally";
        }

        return swipeStatus;
    }
};

Your call to get the data and then build HTML could use an Array instead of concatenating a larger and larger string. Second of all, you only need one call to appendTo:

$.getJSON("People.json", function(data) {
    // The "view model" encapsulating view logic
    var model = new EmployeeStatusViewModel(),
    // The Array to which we append HTML
        content = [];

    $.each(data.People, function(i, peopleObj) {
        var person = model.person = peopleObj[Object.keys(peopleObj)[0]];

        content.push(
            '<tr>',
                '<td>' + person.Op + '</td>',
                '<td class="' + model.getSwipeStatus() + '">' + person.Name + '</td>',
                '<td>' + person.WorkHours + ' </td>',
                '<td>' + person.Start + ' </td>',
                '<td>' + person.End + ' </td>',
                '<td class="' + model.getSwipeStatus() + '">' + person.Clock + '</td>',
                '<td class="' + model.getOffStatus() + '">' + person.OFF + '</td>',
                '<td class="' + model.getOnStatus() + '">' + person.ON + '</td>',
                // build the rest of your columns...
            '</tr>'
        );
    });

    // Join the Array on an empty string and append to a <tbody> only once
    $(content.join("")).appendTo("tbody");
});

Note: I didn't completely flesh out this code example, leaving some of it for you.

Stylistic Changes

Given that I see property names beginning with a capital letter, you may be using .NET on the server? In JavaScript, only Constructor Functions begin with a capital letter, e.g. XMLHttpRequest, Array, Object. Local variables should begin with a lower case letter.

Really, property names should also begin with a lower case letter, but if you are using .NET and serializing an object into JSON, it translates the property names literally, and using PascalCase in .NET for property names is the prevailing pattern, so not much you can do in this regard.

Answer 5

Going off what @Matt Giltaji said, I have removed all the HTML styling and moved it into CSS. I then moved the $(content).appendTo("tbody"); outside of the loop to help performance. Then I threw it through the HTML validator after I tweaked a couple things and it all seems to check out.

$.getJSON("People.json",
function (data) {
content = '<tr>';
content += '<tbody>';
$.each(data.People, function (i, PersonObj) {
var Person = PersonObj[Object.keys(PersonObj)[0]];

content += '<td>' + Person.Op + ' </td>';

// if the person has swiped off but not back on and one hour has elapsed                                  
var swipeStatus;
// if the person has swiped off but not back on and one hour has elapsed  
if (Person.DifHours > 1 && Person.ON === false) {
    swipeStatus = "lateByOneHour";
} else if (Person.OUT !== false) {
    swipeStatus = "notOutYet";
    // If the person has swiped back on from dinner set to lime 
} else if (Person.ON !== false) {
    swipeStatus = "backFromDinner";
    // If the person has swiped of for dinner but 1 hour has not elapsed set to yellow
} else if (Person.OFF !== false) {
    swipeStatus = "outForDinner";
} else if (Person.Clock !== false) {
    // If the person has clocked on set to lime
    swipeStatus = "inNormally";
} else {
    swipeStatus = "defaultStatus";
}
//putting the vales in the table with the swipe status and the infomation
content += '<td class="' + swipeStatus + '">' + Person.Name + '</td>';
// putting values in to the table without formatting 
content += '<td>' + Person.WorkHours + ' </td>';
content += '<td>' + Person.Start + ' </td>';
content += '<td>' + Person.End + ' </td>';
// if the person has not clocked in set the text to white
if (Person.Clock === false) {
    swipeStatus = "NoClock";
}
// if the person is late by 10 mins set to cyan
else if (Person.Late > 10) {
    swipeStatus = "ClockLate";
    // if the person is early by 10 mins set to orange
} else if (Person.Late < -10) {
    swipeStatus = "ClockEarly";
    //if they are ontime with +-10 mins set to lime 
} else {
    swipeStatus = "ClockOntime";
}
//put the infomation in to colum 
content += '<td class="' + swipeStatus + '">' + Person.Clock + '</td>';
//if the person has notswiped 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";
}
//put the infomation in to colum 
content += '<td class="' + swipeStatus + '">' + Person.OFF + '</td>';
// if the person has no swipe back on infomation after swipping off set cell to blank
if (Person.ON === false) {
    swipeStatus = "NoClock";
    // else set the cell to the same unstyled cell
} else {
    swipeStatus = "Blank";
}
//put the infomation in to colum 
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 they didn't go home early and they don't have an out swipe yet then they are still there so just hide the cell
else if (Person.OUT === false) {
    swipeStatus = "NoClock";
    content += '<td class="' + swipeStatus + '">' + Person.OFF + '</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");
});

Answer 6

One trick I enjoy is to separate the information (knowledge) from the decision logic. So without changing your code much:

// What we know
var isOut = Person.OUT !== false;
var hasSwipedBack = Person.ON !== false;
var hasSwipedOff = Person.OFF !== false;
var hasClockedOn = Person.Clock !== false;
var hasNotSwipedBackInOneHour = Person.DifHours > 1 && Person.ON === false;

// What we will do
if (hasNotSwipedBackInOneHour) {
    content += '<td style=" background: red;">' + Person.Name;
} else if (isOut) {
    content += '<td style=" background: red;">' + Person.Name;
} else if (hasSwipedBack) {
    content += '<td style=" background: lime;">' + Person.Name;
} else if (hasSwipedOff) {
    content += '<td style=" background: yellow;">' + Person.Name;
} else if (hasClockedOn) {
    content += '<td style=" background: lime;">' + Person.Name;
} else {
    content += '<td style=" background: Red;">' + Person.Name;
}

This can reduce the need for comments, separates some of the concerns, and can make the code more readable.

Disadvantages: If the determination of knowledge is processor-intensive, and some knowledge is not always used, then calculating it all at the top is inefficient.

We can also reduce a lot of duplication in your code, in keeping with the DRY principle:

// What we will do
var newColor = 'red';
if (hasNotSwipedBackInOneHour) {
    // keep "out" color
} else if (isOut) {
    // keep "out" color
} else if (hasSwipedBack) {
    newColor = 'lime';
} else if (hasSwipedOff) {
    newColor = 'yellow';
} else if (hasClockedOn) {
    newColor = 'lime';
} else {
    // keep "out" color
}

content += '<td style=" background: '+newColor+';">' + Person.Name;

This would be advantageous if at some later point you decide you wanted to set (or just experiment with) background-color or perhaps border instead of the background property.

Finally the above can be represented by a single expression:

// Don't repeat color values, in case we want to change them in future
// Define them just once instead
var IN_COLOR  = 'lime';
var OUT_COLOR = 'red';

// What we will do
var newColor = isOut || hasNotSwipedBackInOneHour ? OUT_COLOR
               : hasSwipedBack                    ? IN_COLOR
               : hasSwipedOff                     ? 'yellow'
               : hasClockedOn                     ? IN_COLOR
               :                                    OUT_COLOR;

Answer 7

I'd suggest starting by using switch statements.

Whenever I see deeply nested / or that many if's I look to see if the language supports a case or switch statement and javascript does.

switch(expression) {
    case n:
        code block
        break;
    case n:
        code block
        break;
    default:
        default code block
} 

http://www.w3schools.com/js/js_switch.asp

Answer 8

You could also start from results and go back to arguments. Then you'll see that there are only 3 outcomes, with "red" color could be easily assumed as default one, which gives us only 2 (!) outcomes and default "in all other cases". I used C syntax (it is pretty much the same with Java and JavaScript, I belive), where you could say "blah != false" as just "blah", and "||" means logical "or". Hopefully the language you're using is also not such complex and allow such expressions.

if (Person.ON || Person.Clock) { //swipped back from dinner or clocked
  content += '<td style=" background: lime;">' + Person.Name;
} else if (Person.OFF) { //swipped off for dinner
  content += '<td style=" background: yellow;">' + Person.Name;
} else { // all other cases - very wrong person
  content += '<td style=" background: red;">' + Person.Name;
}

This is only a first part of your code, consider it like an example of approach.

I also encourage you to use css instead of direct writing of html tags from code. This is that I call "separation of concerns" and it will help you in future to deal with big projects, since you would be able to switch your brain from task to task and wouldn't required to hold a big picture in your brain every time, just a small piece of it. Brains do like it.