Simplify 2 filters on the same array

Question

I have an array of number objects, which can be selected only if the orderable quantity is greater than 0.

I have a select all checkbox that is checked or unchecked accordingly with this function:

self.allSelected = function () {
  return self.shownNumbers().filter(function (number) {
    return number.isSelected();
  }).length === self.shownNumbers().filter(function (number) {
    return number.orderableQty > 0;
  }).length;
}

I can also iterate over the array and return the result:

self.allSelected = function () {
  var s = 0, o = 0;
  for (var i = 0; i < self.shownNumbers().length; i += 1) {
    if (self.shownNumbers()[i].isSelected()) s ++;
    if (self.shownNumbers()[i].orderableQty > 0) o ++;
  }
  return s === o;
}

The second one seems more readable and probably is more efficient since it iterates the array only once. Is there a way to use a single filter, map or other Array.prototype built-in function in order to reduce the code and make it more readable or should I keep the single loop option?

Answer 1

Do either of these implementation really do what you seem to say they do (ensure that all visible items that are orderable are selected?) Perhaps it's hard to tell without understanding whether one can "select" something that is "shown" but not "orderable". If they can, then I think your logic is wrong and you should be comparing that all three number "shown", "selected", and "orderable" are equal.

---

I would consider refactoring. Just as you have methods for shownNumbers() why not add methods for getting all selected items and getting all items with orderable quantity. So, perhaps something like this:

self.allselected = function () {
    return (self.shownNumbers().length === self.filterSelected().length &&
        self.shownNubers().length === self.filterNonzeroQuantity().length);
}

self.getAllSelected = function () {
    return self.shownNumbers().filter(function (number) {
        return number.isSelected();
    }
}

self.getAllOrderable = function () {
    return self.shownNumbers().filter(function (number) {
        return number.orderableQty > 0;
    }
}

Or, if you don't want to refactor then combine the conditions in a single filter. This may or may not work depending on the my very first comments around method logic.

self.allselected = function () {
    var shownNumbers = self.shownNumbers();
    var selectedAndOrdereable = shownNumbers.filter(function (number) {
        return (number.isSelected() && number.orderableQty > 0);
    };
    return selectedAndOrderable.length === shownNumbers.length;
}

---

In either case, don't call shownNumbers() repeatedly in single method. It is unnecessary. Get the result of shownNumbers() and work with it.

---

Consider naming your method something like areAllOrdereablesSelected() that a) indicates to caller that return value will be boolean (similar isSelected()) and b) better hints are what the underlying logic that the boolean represents is. I would think allSelected as a method name would do what your first comparison condition in this method does - get all selected elements.

---

You might be able to optimize for performance and/or memory utilization by implementing loops and counters, but you must decide if it is worth it.