Displaying an item from an array on page load

Question

This piece of Angular code is used to add/remove an item to a favourite array and display it on page load. Firstly I check if I have an array and then create one if not. It also checks the localStorage on page load.

How can this be improved?

$scope.favourites = $localStorage.favourites;

$scope.addExerciseToFavourites = function(exercise) {
  if($scope.favourites === undefined) {
    $scope.favourites = [];
    favouriteToggle(exercise)
  } else {
    favouriteToggle(exercise);
  };
};

function favouriteToggle(exercise) {
  if($scope.favourites.indexOf(exercise.exerciseName) == -1) {
      $scope.favourites.push(exercise.exerciseName);
  } else {
      $scope.favourites.splice($scope.favourites.indexOf(exercise.exerciseName),1);
  };
   $localStorage.favourites = $scope.favourites;
};

Answer 1

It's a good start, but there are some improvements that can be made.

For instance, this chunk:

if($scope.favourites === undefined) {
  $scope.favourites = [];
  favouriteToggle(exercise)
} else {
  favouriteToggle(exercise);
};

Could be written as:

if($scope.favourites === undefined) {
  $scope.favourites = [];
}

favouriteToggle(exercise);

Since you always want to call the favouriteToggle function.

But you can go further, and treat undefined as simply another false'y value like false, null, empty string, and numeric zero:

if(!$scope.favourites) {
  $scope.favourites = [];
}
// ...

Which is synonymous with a common set-default-value-if-false'y idiom:

$scope.favourites = $scope.favourites || [];
// ...

You might also see it written as:

$scope.favourites || ($scope.favourites = []);
// ...

The three code blocks above are all equivalent: If $scope.favourites is false'y, set it to [].

Since it's a simple one-liner, you can just move it out of the function entirely, and then just skip the entire function:

$scope.favourites = $localStorage.favourites || [];

function $scope.favouriteToggle(exercise) {
  if($scope.favourites.indexOf(exercise.exerciseName) == -1) {
      $scope.favourites.push(exercise.exerciseName);
  } else {
      $scope.favourites.splice($scope.favourites.indexOf(exercise.exerciseName),1);
  };
   $localStorage.favourites = $scope.favourites;
};

As for favouriteToggle, I'd name it toggleFavourite instead, just because it sounds more natural. But let's make it little more descriptive, and call it toggleExerciseFavourite.

Secondly, instead of finding the index twice with indexOf, it'd be easier to find it once, and store the result:

$scope.toggleExerciseFavourite = function(exercise) {
  var index = $scope.favourites.indexOf(exercise.exerciseName);

  if(index === -1) {
    $scope.favourites.push(exercise);
  } else {
    $scope.favourites.splice(index, 1);
  }

  $localStorage.favourites = $scope.favourites;
};

Lastly, I don't know Angular, I'm afraid, so I don't know how manipulating the $localStorage object works. But if $scope.favourites is just a reference to it, we can skip the last line, since we're manipulating the same object already:

$scope.toggleExerciseFavourite = function(exercise) {
  var index = $scope.favourites.indexOf(exercise.exerciseName);

  if(index === -1) {
    $scope.favourites.push(exercise);
  } else {
    $scope.favourites.splice(index, 1);
  }
};

Answer 2

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

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