Search code examples
javascriptfor-looptimeline

JavaScript: Optimize function to create a vertical timeline


I created a simple function to iterate through a for loop and display a vertical timeline. Can I get some opinions on what I've created so far?

Are there ways to improve/optimize what I've created so far?

Ideally, what I'm after are ways to minimize duplications, extraneous calls, etc.

Here's my working code: http://jsfiddle.net/bL25b/

function timeline( start, abbr, hours )
{
  var a = 0,
      start,
      abbr,
      hours,
      time = document.getElementById('timeline');

  hours = (!hours) ? 12 : hours;

  for(a = a + start; a <= hours + start; a++)
  {
    if(a > 12)
    {
      time.innerHTML += '<li>' + (a - 12) + ':00 ' +
                          ( abbr == 'AM' ? 'PM' : 'AM' ) +
                        '</li>';

      time.innerHTML += '<li>' + (a - 12) + ':30 ' +
                          ( abbr == 'PM' ? 'AM' : 'PM' ) +
                        '</li>';
    }
    else
    {
      time.innerHTML += '<li>' + a + ':00 ' +
                          ( abbr == 'AM' ? (a == 12) ? 'PM' : 'AM' : 'PM' ) +
                        '</li>';

      time.innerHTML += '<li>' + a + ':30 ' +
                          ( abbr == 'AM' ? (a == 12) ? 'PM' : 'AM' : 'PM' ) +
                        '</li>';
    }
  }
}

timeline( 9, 'AM', 12 );

The function arguments that are accepted:

  • start: start time (0-12)
  • abbr: Am/PM abbreviation
  • hours: number of hours to display

Solution

  • See the updated code:

    function timeline( start, abbr, hours )
    {
        var a = 0,
            abbr = abbr || 'AM',
            hours = hours || 12,
            time = document.getElementById('timeline'),
            timelineHTML = [];
    
        for (a = a + start; a <= hours + start; a++)
        {
            if (a % 12 === 0) {
                abbr = abbr === 'PM' ? 'AM' : 'PM';
            }
    
            timelineHTML.push('<li>' + (a % 12 === 0 ? 12 : a % 12) + ':00 ' + abbr + '</li>');
            timelineHTML.push('<li>' + (a % 12 === 0 ? 12 : a % 12) + ':30 ' + abbr + '</li>');
        }
    
        time.innerHTML = timelineHTML.join('');
    }
    
    timeline( 9, 'AM', 24 );
    

    The most significant change is minifying the DOM operations — we store our future elements in an array and append them at once.

    Secondly, I removed unnecessary and confusing if..else block. The change between 'AM' and 'PM' occures every 12 hours, so a simple modulus operatior will do.

    The part where (a % 12 === 0 ? 12 : a % 12) might be still confusing, but it is kept to display 12:30 AM instead of 0:30 AM. You can change it to short (a % 12) if you want.

    Finally, you used hours = (!hours) ? 12 : hours, while simple hours = hours || 12 is more readable.