3
\$\begingroup\$

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;
            });
        }
    }
};
\$\endgroup\$
  • \$\begingroup\$ FYI "Duplicates in a repeater are not allowed." can be avoided by track by $index. \$\endgroup\$ – Dan Pantry Mar 18 '15 at 9:46
6
\$\begingroup\$

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.

\$\endgroup\$
  • 3
    \$\begingroup\$ delete result.data[key] is how you use delete. It is removing the property given by key from the data object. Also, the bit you are talking about in your 'fourthly' filters inherited properties (e.g. if someone has messed with the Object prototype). \$\endgroup\$ – RobH Mar 17 '15 at 12:56
  • \$\begingroup\$ My mistake on the delete part, then. It looked like an array >.> As for the inherited properties bit, yeah, I know what it does, but it does not make sense here. The results.data will only inherit from Object. You can simply use something like I have suggested to circumvent accessing the prototype. I see no reason as to why you would iterate the object in this way. For example, Object.keys ignores the prototype. \$\endgroup\$ – Dan Pantry Mar 17 '15 at 13:11
  • \$\begingroup\$ Completely agree that it's not necessary. I think Object.keys is IE9+ whereas angular mostly works on IE8 so might not be an option but as you've already shown, don't need to iterate at all. \$\endgroup\$ – RobH Mar 17 '15 at 13:29
  • \$\begingroup\$ That's true but there are polyfills for IE8 for Object.keys available. I think if this was a legitimate concern the code author would have sought out a polyfill. \$\endgroup\$ – Dan Pantry Mar 17 '15 at 13:32
  • \$\begingroup\$ Thanks for the detailed answer. I'm already using loadash 2.4.1. I've tried your changed code, and I think I understand it mostly but for some reason it doesn't work. I'm getting the items I've filtered before in my for / if part of the code again in the list. And what exactly is _? I haven't noticed this before in any other code think. \$\endgroup\$ – burzum Mar 17 '15 at 13:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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