3
\$\begingroup\$

This code loops through an array called $rootScope.watchlist (uses AngularJS). It gets the current iteration of the loop to access that x iteration through $rootScope.watchlist and get the id index. If that matches the variable titleID, then it will splice (remove) that index only and then stop the foreach to stop unnecessarily continuing.

for (var i = $rootScope.watchlist.length - 1; i >= 0; i--) {
    if($rootScope.watchlist[i]['id'] == titleID) {
        $rootScope.watchlist.splice(i, 1);
        break;
    }
}

Is there a better way of doing such? Possibly using .some()?

\$\endgroup\$
  • \$\begingroup\$ What exactly are you using this for in practice? \$\endgroup\$ – BenC Sep 9 '17 at 1:02
2
\$\begingroup\$

A while back, I went through these exercises about functional programming in JS and since then I strive to use those techniques whenever possible. Bear in mind that functional techniques can lead to slower code, since a function call is made for each iteration instead of just running code within a loop block, but hopefully it is easier to work with each array element in that fashion - e.g. would you read/type if($rootScope.watchlist[i]['id'] == titleID) { or if(element['id'] == titleID) {?

After reading Igor's answer as well as this article about break with respect to functional programming, I believe the following should work:

$rootScope
.watchlist
.slice()         //  copy before reverse(), because reverse() is mutating
.reverse()
.some(function(element, i) {
    if(element['id'] == titleID) {
        $rootScope.watchlist.splice($rootScope.watchlist.length - 1 - i, 1);
        return true;
    }
});

Edit:

Igor suggests simplifying that callback passed to some(). You can use an ES-6 arrow function and a ternary operator to eliminate a few lines, like this:

.some((element, i) => (element['id'] == titleID)? $rootScope.watchlist.splice($rootScope.watchlist.length - 1 - i, 1) : false);

See a demonstration in this plunker. Click the button labeled remove watcher and see how the watcher is removed. This code presumes $rootScope.watchlist is equivalent to $rootScope.$$watchers...

\$\endgroup\$
  • 1
    \$\begingroup\$ You showed a really useful technique! Nice improvement, Sam! I'd still compress the .some()'s body to this: .some((element, elementIndex) => element['id'] != titleId ? false : ($rootScope.watchlist.splice($rootScope.watchlist.length - elementIndex - 1, 1), true)) \$\endgroup\$ – Igor Soloydenko Sep 9 '17 at 4:08
1
\$\begingroup\$

Here's what functional style might look like. Unfortunately, it requires index recalculation, and overall more verbose and complicated, even though it tries to go the ".some()" way. :)

I believe it's one of those cases where imperative code keeps things simple.

const indexFromRear =
  $rootScope
    .watchlist
    .slice()         //  copy before reverse(), because reverse() is mutating
    .reverse()       //  reverse, since the original code searches rear-to-front
    .findIndex(element => element['id'] == titleID);

if (indexFromRear >= 0) {
  const indexFromFront = $rootScope.watchlist.length - indexFromRear - 1;
  $rootScope.watchlist.splice(indexFromFront, 1);
}
\$\endgroup\$

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.