Most things, Flambino@Flambino has already mentioned most things. Here are some additions and an alternative solution:
1) Generals
- 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 astonishmentPrinciple 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 principleSingle 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.
2) The actual code
- The actual code
Flambino@Flambino already pointed out, that your initialization could be simplified to scope.favs=scope.favs||[];
this.
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);
}
}
var f=scope.favs.filter(function(x){ return x!== exercise });
This one This:
var f=scope.favs.filter(function(x){ return x!== exercise });
tries to filter out the excerciseexercise. 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.