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 am building a ticket management system for a state server we use at work. Back end is node.js with multiple front ends. I am rewriting my back end to be more maintainable as the scope grows.

For this particular function I start with this:

[
    {
        "id": 4993,
        "ticket": "A142032570",
        "sort": "07/22/14"
    },
    {
        "id": 5060,
        "ticket": "A142043078",
        "sort": "07/23/14"
    }
]

with many more entries than shown here. My function takes this data and creates this:

[
    {
        "date": "07/22/14",
        "tickets": [
            {
                "id": 4993,
                "ticket": "A142032570"
            }
        ]
    },
    {
        "date": "07/23/14",
        "tickets": [
            {
                "id": 5060,
                "ticket": "A142043078"
            }
        ]
    }
]

The function used is:

function sortMyTicketsByDate(data){
    var arr = [];
    for(var o in data){
        arr.push(data[o].sort);
    }
    arr = _.uniq(arr);
    for(var i in arr){
        arr[i] = {date: arr[i]};
    }
    for(var d in arr){
        console.log(arr[d]);
        arr[d].tickets = [];
        var elem = arr[d]['date'];
        for(var o in data){
            //data[o].sort, ticket, id
            if (elem === data[o].sort) {
                arr[d]['tickets'].push({id: data[o].id, ticket: data[o].ticket});
            };
        }
    }
    return arr;
}

What improvements can be made to sortMyTicketsByDate to be more efficient/understandable?

share|improve this question
4  
Well, if arr is an array, you're using the wrong loop, instead of for..in you should use a regular for. –  elclanrs Jul 31 at 20:37
    
@elclanrs is that more efficient for the JS engine? Or is this a style best practice? –  xandout Jul 31 at 20:41
3  
No, it is just wrong, for..in is for looping objects, and object keys have no order; while an array is an object, if you want to loop it as an array, you need to use a for loop, because it will keep the order, while the order of for..in is not guaranteed by the spec. –  elclanrs Jul 31 at 20:42
3  
You should parse your dates to actual dates so "07/22/14" < "07/21/15". Comparing strings are done via lexical comparisions –  megawac Jul 31 at 20:44
2  
@xandout Nope but you can use the time in milliseconds if you wanted –  megawac Jul 31 at 21:05

3 Answers 3

  • As mentioned in the comments by @elclanrs, you should not use for...in to loop through arrays.

  • As mentioned by @megawac, lexical comparison will not produce the results you expect. So you'll need to parse those strings.

  • Use more descriptive names. data should probably be tickets, arr could be dates. Nothing should be just o or d. It makes the code much, much harder to read.

  • Remember to remove all console.log calls in production code (or do a check to make sure that console exists, and has a function called log before you try calling it - otherwise things break)

Since you seem to be using underscore.js, you should look at the groupBy function

var grouped = _.groupBy(tickets, function (ticket) { return ticket.date });

The above will create an object with the date strings as keys, and arrays of tickets as values. Pretty much the same as the first half of your current function.

But we can do more with underscore. All of it actually:

function sortMyTicketsByDate(tickets) {
  return _.chain(tickets)
    .groupBy(function (ticket) { return ticket.sort })
    .sortBy(function (group, date) {
      var parts = date.split(/\/0?/);
      return new Date(parts[3], parts[1] - 1, parts[2]);
    }) 
    .map(function (group, date) { return { date: date, tickets: group }; })
    .value();
}

Caveat emptor: The major difference between the above function and your original one, this one returns the same objects it was given, whereas yours return copies. So any change made to the tickets afterward are being made to the originals.

That also means that the sort property remains (whereas you just don't copy it)

To avoid these points (if you really need to, which I very much doubt you do), you can do this:

function sortMyTicketsByDate(tickets) {
  return _.chain(tickets)
    .map(_.clone) // copy objects
    .groupBy(function (ticket) { return ticket.sort })
    .sortBy(function (group, date) {
      var parts = date.split(/\/0?/);
      return new Date(parts[3], parts[1] - 1, parts[2]);
    }) 
    .map(function (group, date) { return { date: date, tickets: group }; })
    .each(function (group) { // remove sort property
      _.each(group.tickets, function (ticket) { delete ticket["sort"] });
    })
    .value();
}
share|improve this answer

All right there are a couple things that are off about your function. First off, I'd claim it's not particularly reusable and it took me a decent while to understand what it was doing...

  1. as I mentioned in comments you compare dates as strings. In JS Strings are compared lexically so '7' > '10' I assume you don't want "07/22/14" > "07/21/15". Convert these to a Date first which can be compared by time in milliseconds

    new Date("07/22/14") < new Date("07/21/15")
    

    (The <, >, etc operators convert the date to its time in milliseconds and compare the integers, try +new Date)

  2. You use for x in arr which is incorrect in JS for various reasons

Seeing you have underscore on the page, why not use the provided sortBy function. It's pretty nice. Note this isn't the most efficent way to write this code, but it's very easy to read.

function sortMyTicketsByDate(data) {
    return _.chain(data)
    .groupBy('sort')
    .map(function(tickets, date) {
        return {
            "date": date,
            "tickets": tickets
        };
    })
    .sortBy(function(item) {
        return Date.parse(item.date);
    })
    .value();
}

This will produce the same result as your code, except it handles dates and I'd argue it's a great deal more readable.

share|improve this answer
2  
Careful with Date.parse on ambiguous formats like this (cf. MDN) –  Flambino Jul 31 at 21:23

It seems like you are using underscore library. so why you need to do it manually?

var groupeddata = _.groupBy(data, function (row) {
var from = row.sort.split("/");
var dateparse = new Date(from[2], from[0] - 1, from[1]);
return dateparse;
});

I hope this will simplify your code

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.