current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

I wanted to get some eyes on some code that I wrote, and see if anyone can help improve what I did. Just looking for some constructive feedback to help make my code more performant, and more elegant.

From a high level my code needs to accomplish the following:

Create a function that takes an array of objects that each represent an event. Each object in the array has a start and end time represented in minutes. The start and end times are represented as minutes after 9am. For example {start:30, end: 60} represents an event that starts at 9:30am and ends at 10am. The function takes the objects, and plots them as events on a vertical timeline. Events that have crossover will split the container in half, while events that don’t crossover will take the whole container width.

Here is the JS that I wrote to accomplish the above task:

//sample data - array of objects that represent events
var myArray = [
{start: 30, end: 130},
{start: 140, end: 147},
{start: 145, end: 155},
{start: 150, end: 175},
{start: 200, end: 250},
];

var myFunc = function(a) {
  //sort the input array from earliest start time to latest. Items with identical start times will compare end times - the longer duration item resolves with lower index. Assumes no exact duplicate items.
  var sortedDates = a.sort(function(a,b) {
    var caseOne= (a.end>b.end) ? -1 : 1,
        caseTwo= a.start - b.start;

    return (a.start===b.start) ? caseOne : caseTwo;
  });

  for(var i=0; i<sortedDates.length; i++) {
    var currentItem = sortedDates[i],
        itemDuration = currentItem.end-currentItem.start,
        itemTop = currentItem.start,
        prevItem = sortedDates[i-1],
        nextItem = sortedDates[i+1],
        newDiv = document.createElement('div');

    //set a default direction to each item
    currentItem.direction = 'left';

    //determine which items overlap and set a property to indicate
    if(nextItem !== undefined && (currentItem.end - nextItem.start > 0)) {
      currentItem.overlap = true;
      nextItem.overlap = true;
    }

    //ensure items flow in UI by staggering overlapping items
    if(prevItem !== undefined && (prevItem.direction === 'left')) {
      currentItem.direction = 'right';
    }

    //set class names on new DOM element based on overlap
    if(currentItem.overlap === true) {
      if(currentItem.hasOwnProperty('direction')) {
        newDiv.setAttribute('class', 'split '+currentItem.direction);
      }
      else {
        newDiv.setAttribute('class', 'split');
      }
    }

    //set the size and position based on computed duration and start time
    newDiv.setAttribute('style', 'height:'+itemDuration+'px; top:'+itemTop+'px');

    //insert new element into DOM
    document.getElementById('stuff').appendChild(newDiv);
  }
};

Here is a rough working example.

share|improve this question

1 Answer 1

up vote 0 down vote

Comments are in the code:

var myArray = [
    { start: 30, end: 130 },
    { start: 140, end: 147 },
    { start: 145, end: 155 },
    { start: 150, end: 175 },
    { start: 200, end: 250 }
];

var myFunc = function (dates) {

    // sort the input array from earliest start time to latest. Items with
    // identical start times will compare end times - the longer duration
    // item resolves with lower index. Assumes no exact duplicate items.
    var sortedDates = dates.sort(function (a, b) {
            // replaced the 
            // var caseOne = (a.end > b.end) ? -1 : 1,
            //     caseTwo = a.start - b.start;
            // return (a.start===b.start) ? caseOne : caseTwo
            // with a shorter short cut, considering the start value are the same,
            // it evaluates to falsy (0) and the other part is taken, if evaluated 
            // to truthy, the value of the first difference is taken.
            return a.start - b.start || b.end - a.end;
        }),
        factor = 2; // for some more space

    // i change the for (...) loop to the more handy Array.forEach loop
    sortedDates.forEach(function (item, i, o) {

        var itemDuration = factor * (item.end - item.start), // applied factor
            itemTop = factor * item.start,                   // applied factor
            prevItem = o[i - 1],
            nextItem = o[i + 1],
            newDiv = document.createElement('div');

        // set a default direction to each item
        item.direction = 'left';

        // determine which items overlap and set a property to indicate
        // changed the comparison nextItem !== undefined to simply nextItem
        // and the subtraction to a direct comparison
        if (nextItem && item.end > nextItem.start) {
            item.overlap = true;
            nextItem.overlap = true;
        }

        // ensure items flow in UI by staggering overlapping items
        // changed the comparison prevItem !== undefined to simply prevItem
        if (prevItem && prevItem.direction === 'left') {
            item.direction = 'right';
        }

        // set class names on new DOM element based on overlap
        // replaced item.overlap === true with item.overlap because if not set
        // then it evaluates to false and if set, but false the it stays false, 
        // otherwise if set and set to true, it evaluates true, then no furter
        // comparison is needed

        // the other change is the comparison of item.hasOwnProperty('direction')
        // here logical or is applied, because item.direction evaluates to false
        // if not set, with the || '' we get an empty string, but if set, then
        // only this result becomes the value
        item.overlap && newDiv.setAttribute('class', 'split ' + item.direction || '');

        //set the size and position based on computed duration and start time
        newDiv.setAttribute('style', 'height:' + itemDuration + 'px; top:' + itemTop + 'px');
        // to see some information
        newDiv.innerText = i + ': ' + item.start + ' - ' + item.end;

        //insert new element into DOM
        console.log(newDiv);
        document.getElementById('calendar').appendChild(newDiv);
    });

};

myFunc(myArray);
#calendar {
    height: 720px;
    width: 100%;
    position: relative;
}
#calendar div {
    position: absolute;
    width: 100%;
    min-height: 5px;
    background: brown;
}
#calendar .split {
    width: 50%;
    background: pink;
}
.right {
    right: 0;
}
.left {
    left: 0;
}
<div id="calendar"></div>

share|improve this answer
    
You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. – Quill Aug 17 at 12:18
    
it's an interesting approach to explain what you did in code comments. Unfortunately, this is not easy to read and understand. Especially at parts where your comments are mixed with original comments in the code. Instead of embedding your comments, it would be better to extract the relevant snippets to highlight them, and how you corrected them – janos♦ Sep 25 at 19:47

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.