1
\$\begingroup\$

I have the following code, that groups similar todos into an array of arrays, using lodash (https://lodash.com). Here is a plnkr http://plnkr.co/edit/t4J5bKGjMlQxW2z9Zh7x?p=preview (open your console and look into script.js)

function Todo(value) {
  this.value = value;
  this.isSimilar = function(todo) {
    return this.value == todo.value;
  };
}

var _data = [
  new Todo("hello"),
  new Todo("world"),
  new Todo("foo"),
  new Todo("bar"),
  new Todo("hello")
];

var processedTodos = [];
var groups = _.compact(_.map(_data, function(todo) {
  if(processedTodos.indexOf(todo)>-1) {
    return;
  }
  var similarTodos = _.filter(_data, function(todo2) {
    return todo!==todo2 && todo.isSimilar(todo2);
  });
  similarTodos.unshift(todo); // prepend todo to front of array
  [].push.apply(processedTodos, similarTodos);
  return similarTodos;
}));

console.log(groups);

groups contains an array of arrays, where similar todos are in the same array.

For example, when I input this (these are JS objects, but for the sake of readability in JSON):

[
  {"value":"hello"},
  {"value":"world"},
  {"value":"foo"},
  {"value":"bar"},
  {"value":"hello"}
]

It returns this:

[
  [{"value":"hello"},{"value":"hello"}],
  [{"value":"world"}],
  [{"value":"foo"}],
  [{"value":"bar"}]
]

It groups together similar objects.

But the whole code doesn't feel right, there must be a better way to do this!

Another bad thing is that I need an additional array to store the already checked todos (which avoid me duplicate arrays like [todo1,todo2] and [todo2,todo1].

Last but not least, I have to remove undefined values from the array with _.compact, because this line returns undefined when the todo was already processed:

  if(processedTodos.indexOf(todo)>-1) {
    return;
  }

Any idea how to improve this code?

\$\endgroup\$
2
  • \$\begingroup\$ Can you add a sample input and its corresponding output? \$\endgroup\$
    – Joseph
    Commented Nov 24, 2015 at 14:28
  • \$\begingroup\$ I've added some input and output, and there is also a plnkr link, if you missed it: plnkr.co/edit/t4J5bKGjMlQxW2z9Zh7x?p=preview \$\endgroup\$
    – 23tux
    Commented Nov 24, 2015 at 14:45

1 Answer 1

3
\$\begingroup\$

First, using a constructor is overkill. Your Todos don't use inheritance nor share methods (the method is created per instance, thus not shared). You can simply have a factory (a function that returns an object). isSimilar could be just a helper function that has nothing to do with Todo.

function createTodo(value){
  return {
    value: value,
  };
}

var _data = [
  createTodo("hello"),
  createTodo("world"),
  createTodo("foo"),
  createTodo("bar"),
  createTodo("hello")
];

So now, you have a list of objects. You can then use reduce and build up a hash of objects with the value as their key, and an array as the value.

var todosByValue = _data.reduce(function(hash, todo){
  if(!hash.hasOwnProperty(todo.value)) hash[todo.value] = [];
  hash[todo.value].push(todo);
  return hash;
}, {});

// now we have something like:
{
  hello: [{ value: 'hello' }, { value: 'hello' }],
  world: [{ value: 'world' }],
  ...
}

All you need to do is just pull the values out to an array using Object.keys and array.map.

var groupedByTodo = Object.keys(todosByValue).map(function(key){
  return todosByValue[key];
});

In all, it should simply look like this:

function createTodo(value){
  return {
    value: value,
  };
}

var _data = [
  createTodo("hello"),
  createTodo("world"),
  createTodo("foo"),
  createTodo("bar"),
  createTodo("hello")
];

var todosByValue = _data.reduce(function(hash, todo){
  if(!hash.hasOwnProperty(todo.value)) hash[todo.value] = [];
  hash[todo.value].push(todo);
  return hash;
}, {});

var groupedByTodo = Object.keys(todosByValue).map(function(key){
  return todosByValue[key];
});
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.