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

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

1) Generals

  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 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

  1. 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.

Most things, Flambino has already mentioned. 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.

2) The actual code

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 tries to filter out the excercise. 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.

@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.

Source Link
Thomas Junk
  • 2k
  • 14
  • 18

Most things, Flambino has already mentioned. 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.

2) The actual code

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 tries to filter out the excercise. 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.