Skip to main content
2 of 2
added 46 characters in body
Jamal
  • 34.9k
  • 13
  • 133
  • 237

@Flambino has already mentioned most things. Here are some additions and an alternative solution:

  1. Generals

Besides I do not like excessive $ - but that's a matter of taste. Your functions are actually doing too much. addElement does in fact not only add an element, it also removes it. This violates the Principle of the Least Astonishment. Then, you are initializing your favs inside this function, which has nothing to do with toggling. This violates clearly the Single Responsibility Principle: Do only "one" thing in a function. And at least, you are storing the result to localStorage. This would also be better done outside.

  1. The actual code

@Flambino already pointed out that your initialization could be simplified to scope.favs=scope.favs||[];.

I do not know any AngularJS, so I boiled it down to this:

    function toggleExcercise(exercise){
        var f=scope.favs.filter(function(x){ return x!== exercise });
        if(f.length < scope.favs.length) {
            scope.favs=f;
        } else {
            scope.favs.push(exercise);
        }
    }

This:

    var f=scope.favs.filter(function(x){ return x!== exercise });

tries to filter out the exercise. For that, the array is run through once.

If you are using indexOf and splice the array is run through more than once worst case. Here is the Fiddle to play with.

Thomas Junk
  • 2k
  • 14
  • 18