2
\$\begingroup\$

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;
};
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

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);
  }
};
\$\endgroup\$
1
  • \$\begingroup\$ Great Flambino, thank you very much for taking the time to do this for me. It is working great. \$\endgroup\$
    – Taylorsuk
    May 3, 2015 at 16:52
1
\$\begingroup\$

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

\$\endgroup\$
5
  • \$\begingroup\$ Many thanks for taking the time Thomas - it takes me some time to read that quite confusing for a amateur. \$\endgroup\$
    – Taylorsuk
    May 3, 2015 at 16:50
  • \$\begingroup\$ I am open for questions ;) \$\endgroup\$ May 3, 2015 at 17:04
  • 1
    \$\begingroup\$ So I think its this line that is confusing: var f=scope.favs.filter(function(x){ return x!== exercise }); there is a lot going on within one line also not seen a function within a variable I don't think. And also using x which I don't see where that comes from? guess its just the shorthand I can't get my head around. \$\endgroup\$
    – Taylorsuk
    May 3, 2015 at 17:14
  • 1
    \$\begingroup\$ The function I've used is: developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/…. It takes another function (which I defined anonymous inline ) as it's argument, which describes the filtering rule x!==exercise. X is in this case arbitrary. I chose it out of habit - like in maths f(x). \$\endgroup\$ May 3, 2015 at 17:18
  • \$\begingroup\$ To translate it into natural language: take the array of favs, filter it with the rule (taking every x and compare it to not beeing exercise - if true return the element else skip). Or: filter everything beeing equal to exercise out. \$\endgroup\$ May 3, 2015 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.