patternjavascriptModerate
Ticket management system
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:
with many more entries than shown here. My function takes this data and creates this:
The function used is:
What improvements can be made to
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
-
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.
-
Remember to remove all
Since you seem to be using underscore.js, you should look at the
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:
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
To avoid these points (if you really need to, which I very much doubt you do), you can do this:
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 functionvar 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.