Filter an array of objects by an array in AngularJS

Question

This code is the whole logic for the AngularJS ui-select lib. I'm not (yet) a JS pro and still learning AngularJS as well but I'm pretty sure this can be done a little more efficient? I'm talking especially about the search: part of the object. In php I would probably do something like array_diff. Any critiques are welcome!

The reason for the selectedIds: [] is that I use ng-repeat to render a list of hidden fields with name="users[]" to be able to fetch the list as a php array on the server side. I'm not suing Angular as a single page app here, I'm just using it for all the widgets but the data itself is submitted with the whole page as POST.

The for (var key in result.data) is done to filter already selected ids out of the array. It was the most easy solution to avoid the error Duplicates in a repeater are not allowed.. Sure I could pass the already existing ids to the lookup GET request but I think this works fine on the client side as well.

$scope.usersList = {
    items: [],
    selected: [],
    selectedIds: [],
    extractIds: function() {
        $scope.usersList.selectedIds = [];
        for (var i = 0; i < $scope.usersList.selected.length; i++) {
            $scope.usersList.selectedIds.push($scope.usersList.selected[i].id);
        }
    },
    select: function($item, $model) {
        this.extractIds();
    },
    remove: function($item, $model) {
        this.extractIds();
    },
    search: function(term) {
        if (term.length > 2) {
            UsersService.lookup(term).then(function(result) {
                for (var key in result.data) {
                    if (result.data.hasOwnProperty(key)) {
                        if ($scope.usersList.selectedIds.indexOf(result.data[key].id) != -1) {
                            delete result.data[key];
                        };
                    }
                }
                $scope.usersList.items = result.data;
            });
        }
    }
};

Answer 1

welcome to CodeReview :-)

Firstly, I would mention that your $item and $model parameters should just be item and model. The dollar-sign prefix is conventionally reserved for official vendor libraries from Angular, and should not be used in your code to prevent confusion. I initially thought $model was a derivative of ngModel!

Secondly, do you need selectedIds? Can you not just use a function to extract those IDs? You do not need to watch a seperate array of IDs.

Thirdly - delete result.data[key] - this is not how you use delete. You use delete to remove properties from objects, not key/value pairs from collections. Instead, you should just set it to undefined.. with that said, you don't even need to use delete here anyway.

Fourthly.. what the hell is going on with for(key in result.data) if(result.data.hasOwnProperty(key)... that code is iterating the data then checking that the data has that property. wat. This works for circumventing prototypal inheritance, however you can just as easily use Object.keys(). Not to mention that the result of a $http request is only going to inherit from Object.

With that said, here's a refactor of your code. You will need lodash for this code to work, but no worries - lodash is probably the most handy library you will ever come across in JS. I use it on every project these days.

$scope.usersList = {
  items: [],
  search: function(term) {
    if(term.length > 2) {
      UsersService.lookup(term)
        .then(function(xhr) {
          return _.reject(xhr.data, function(item) {
            return _.contains($scope.usersList.items, item);
          });
        })
        .then(function(data) {
          $scope.usersList.items = data;
        });
    }
  }
};

I've removed the select, remove and extractId functions as they do nothing in my example (and they did nothing anyway!)

This could definitely be refactored even better in my opinion, but I would need more scope on the actual problem to do that. For example, there's no need to use the $scope here aside from exposing the results to the view. This would be much better done if it was encapsulated inside of a function and only the result was exposed. This should really be in two separate classes at least.