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\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.