0
\$\begingroup\$

I am using the function inputSetup to prepare an array for a multi-select directive. I am passing in an array with the existing selections ($scope.item.categories), an array with all the available categories, and a string the carries the context of the values "category".

  1. Formatting the array to add the value "ticked" = true; on each object, so that the multi-select understand which options are selected.

  2. Concatenating the arrays.

  3. Sorting the arrays.

  4. Using a for loop to iterate through the array and removing any duplicates that have the same "category" key... then removing the one that has "ticked" = false.

This code feels like it can be improved.

  1. Should the .concat and .sort be chained together?
  2. Should the for loop be replaced with an array.filter()?

$scope.categories = inputCategories($scope.item.categories, $scope.settings.categories, "category");

function inputSet (input, settings, category) {
  var updatedSettings = [];

  angular.forEach(input, function(obj) {
    var setting = { "ticked": true };
    setting[category] = obj;
    updatedSettings.push(setting);
  });

  var list = updatedSettings.concat(settings);

  list.sort(function(a, b) {
    return (a[category] > b[category]) - (a[category] < b[category]);
  });

  for ( var i = 1; i < list.length; i++ ){
    if(list[i-1][category] == list[i][category]) {
      list.splice(i,1);
    }
  }
  return list;
};
\$\endgroup\$
1
\$\begingroup\$

Here's my take on your code. Longer, but in my opinion, easier to follow since it is written as a chain of operations on arrays.

In addition, I use array methods, which easily chain together. Their callbacks are moved out for clarity. They're also written in a way that a function doesn't use outside variables. Makes your code more predictable and easily modifiable without dragging along other stuff.

// The first phase of your code is just mapping obj into another form. Use map().
function inputMapper(obj) {
  var setting = {};
  // Using the bracket notation to keep it uniform.
  setting['ticked'] = true;
  setting[category] = obj;
  return setting;
}

// Removing items first will make further operations iterate less.
// Additionally, we use reduce to carry around the data, and the already-found categories
function duplicateCategoryRemover(carry, item) {
  if (!~carry.categoriesFound.indexOf(item.category)) {
    carry.items.push(item);
    carry.categoriesFound.push(item.category);
  }
  return carry;
}

// Nothing special, just your sorter moved out.
function categorySorter(a, b) {
  return (a[category] > b[category]) - (a[category] < b[category]);
}

// Now everything's in place, let's do the magic.
function inputSet(input, settings, category) {
  return input.map(inputMapper)
              .concat(settings)
              .reduce(duplicateCategoryRemover, { items: [], categoriesFound: [] })
              .items
              .sort(categorySorter);
};
\$\endgroup\$
  • \$\begingroup\$ Your approach is high level art, compared to my children's drawings... and I have learned over ten theoretical improvements to programming. The one that stands out the most is that dividing the code into such a neat separation of function and action - really means that although longer (slightly), it is so much move valuable, maintainable and reusable. Thank you @Joseph the Dreamer \$\endgroup\$ – John Spiteri Aug 2 '15 at 3:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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