Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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

        });
    });
share|improve this question
3  
Could you please describe this code a bit? –  Jamal Aug 20 at 18:10
1  
No HTML escaping? I suppose it's guaranteed that Person.Name won't have a < in it (it isn't user input)? –  Brilliand Aug 20 at 20:06
2  
@JoshKirkpatrick Although it isn't a security problem in this case, I'd still recommend not treating data as HTML if it isn't meant to be HTML (in this case, construct the td element and then set its text, rather than concatenating the text into an HTML string). –  Brilliand Aug 20 at 20:39
1  
Since you are building a lot of the HTML, you could check out a templating library like Mustache.js or Handlebars.js. These libraries have their own way of dealing with conditionals and might simplify a big chunk of this process. –  Jonny Sooter Aug 20 at 21:46
2  

8 Answers 8

up vote 35 down vote accepted

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.

share|improve this answer
2  
I want to note that it is entirely valid to leave certain tags unclosed, depending on the context. For example, <tbody>, <tr> and <td> never need to be explicitly closed; they are automatically closed when either their containing element (<table>, <tbody>, or <tr>, respectively) is closed, or when a suitable alternative (like <thead>, another <tr>, or <th>, respectively) is opened. –  Schism Aug 20 at 18:01
    
It wouldn't pass the Html validator right? –  TopinFrassi Aug 20 at 18:12
6  
While it's true that you don't need to close such tags it is probably better as a novice to err on the side of caution. If as a matter of habit you always close a block tag explicitly you don't have to remember which ones are automatically closed. –  Alfred Armstrong Aug 20 at 19:01
1  
@TopinFrassi, the following passes W3C's validator: <!DOCTYPE html> <html> <head> <title>Title</title> </head> <body> <table><tbody><tr><td></table> </body> –  Dancrumb Aug 20 at 19:15
1  
Does it get any realler than this: w3.org/html/wg/drafts/html/master/syntax.html#the-doctype? –  Dancrumb Aug 20 at 20:55

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
    
share|improve this answer
    
Couple of nits: $.each will iterate over a non-Array object just fine. It just passes the key to the callback instead of the index. More importantly, you should never depend upon an absolute index into the array returned by Object.keys(), because ordering is not guaranteed. It is guaranteed to be the same as for..in ordering, but neither should be expected to be in any particular order. –  codelahoma Aug 21 at 1:00
    
Don't simply concatenate data into HTML. You're not doing any escaping... Use .text() instead. –  Brad Aug 21 at 3:03
    
@codelahoma I suspect data.People is an array instead of an object since the OP chose i as the key/index instead of key and ignores it in the callback. As for using Object.keys(PersonObj)[0], this is what the OP used. Without example JSON, we are left to guess at the intent. –  David Harkness Aug 21 at 9:39
    
@Brad It would be interesting to see the performance cost of building the table DOM elements detached from the document to be able to use .text() in lieu of DOM-free escaping functions. –  David Harkness Aug 21 at 9:42
    
@DavidHarkness Oh it's definitely slower, but what's your point? Your code here is plainly incorrect and will fail as soon as someone puts in a string in one of those variables with HTML in it. Plus, you don't want someone putting <script src="somethingEvil.js"></script> in for their user name. –  Brad Aug 21 at 12:56

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.

share|improve this answer
    
The fallback is resulting to a background red, which in this case Person.DifHours<=1 and Person.OUT === false will result in red. Just check the default value of the variable color. It is red. So default (fallback value) is red in this case –  V31 Aug 20 at 16:31
    
Wanted to say that the OP can club if statements with and/or as in the color code can be the same for some conditions can elaborate the answe if required –  V31 Aug 20 at 16:38
    
@Schism, the author did write something like this, it is great you point out bugs, but -1 seems harsh –  konijn Aug 20 at 16:41
    
@konijn If a review changes the logic of the code I think it's deserving of a -1. Buuut, I had missed the first line; I made a relatively minor edit so that I'd be able to change my vote to a +1. –  Schism Aug 20 at 16:45
    
@Schism: Yep that was the actual meaning what I wanted to convey :) –  V31 Aug 20 at 16:45

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.

share|improve this answer

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");
});
share|improve this answer

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;
share|improve this answer

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

share|improve this answer
    
switch and all it's equivalents are discouraged as hard to read. Therefore many languages just don't have this concept. –  ElmoVanKielmo Aug 21 at 13:11
    
Like most languages, JavaScript's switch accepts a value for each case, not another expression. –  David Harkness Aug 22 at 5:43

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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