Ticket management system

Question

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?

Answer 1

• 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();
}

Answer 2

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.

Answer 3

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