HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptModerate

Ticket management system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managementsystemticket

Problem

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?

Solution

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

Code Snippets

var grouped = _.groupBy(tickets, function (ticket) { return ticket.date });
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();
}
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();
}

Context

StackExchange Code Review Q#58692, answer score: 10

Revisions (0)

No revisions yet.