2
\$\begingroup\$

This is my first open source release and I'd like to get some feedback on my code styling/organization. Implementation tips are appreciated as well.

The code is for an AngularJS select box that allows you to select from a list of values, or enter in free text.

The control can be configured to:

  • compare against an attribute of the source items.
  • return a specific attribute (EG for searching names, but returning an id).
  • return the source object or the value (generating a object in the case of free text input.

GitHub

Example usage:

HTML

<body ng-app="test">

<script type="text/javascript">
  angular.module("test", ['fzSelect'])
  .controller("test", function($scope){
    $scope.myItems = [{name: 'one', value: 1}, {name: 'two', value: 2}, {name: 'three', value: 3}];
    $scope.myValue = { value: "something" };
  })
</script>

<div style="width: 600px;" ng-controller="test">
  <h1> {{myValue.value}} </h1>
  <div fz-select fz-select-items="myItems" 
       fz-match-attribute="name" 
       fz-return-attribute="value" 
       fz-return-objects="false"
       ng-model="myValue.value" />

  </div>
</body>

Angular Code

angular.module( "fzSelect", [] )
.directive( "fzSelect", ['$filter', '$timeout', '$parse',
  function($filter, $timeout, $parse){
    return {
      restrict: 'EA',
      // require: ['ngModel', 'fzSelectItems'],
      template: 
'<div class="input-group" >'+
  '<input class="form-control" ng-model="searchString"></input>'+
  '<span class="input-group-btn">'+
    '<button class="btn btn-primary" ng-click="showAll()" >&#9660;</button>'+
  '</span>'+
'</div>'+
'<div class="fz-select-results-container" ng-if="resultsVisible.value"> '+
  '<div  class="fz-select-results-row" '+
    'ng-repeat="item in filteredItems" '+
    'ng-click="resultItemClicked(item)">{{getItemDisplayString(item)}}</div>'+
'</div>',
      link: function($scope, element, attrs){

        angular.element(element).addClass('fz-select-component');

        var itemsGetter = $parse(attrs.fzSelectItems);
        var valueGetter = $parse(attrs.ngModel);
        var valueSetter = valueGetter.assign;

        var itemAttributeName = null;
        var itemAttributeGetter = null;
        if( attrs.hasOwnProperty('fzMatchAttribute') ){
          itemAttributeGetter = $parse(attrs.fzMatchAttribute);
          itemAttributeName = attrs.fzMatchAttribute;
        }

        var itemReturnAttributeName = "value";
        var itemReturnAttributeGetter = null;
        if( attrs.hasOwnProperty('fzReturnAttribute') ){
          itemReturnAttributeGetter = $parse(attrs.fzReturnAttribute);
          itemReturnAttributeName = attrs.fzReturnAttribute;
        } else if( attrs.hasOwnProperty('fzMatchAttribute') ){
          itemReturnAttributeGetter = $parse(attrs.fzMatchAttribute);
          itemReturnAttributeName = attrs.fzMatchAttribute;
        }


        var returnObjects = false;
        if( attrs.hasOwnProperty("fzReturnObjects") ){
          returnObjects = attrs.fzReturnObjects == "true";
        }

        $scope.items = itemsGetter($scope);
        $scope.searchString = valueGetter($scope);
        $scope.filteredItems = [];
        $scope.resultsVisible = {value: false};
        $scope.selectedValue = null;
        var valueWasSelected = false;

        $scope.showResults = function(show){
          $timeout(function(){
            $scope.resultsVisible.value = show;
          });
        };

        $scope.getItemDisplayString = function(item){
          if( itemAttributeGetter != null  ){
            return itemAttributeGetter(item); 
          }else{
            return item;
          }
        };

        $scope.showAll = function(){
          $scope.filteredItems = $scope.items;
          $scope.showResults(true);
        };

        $scope.updateSourceValue = function(){
          if($scope.selectedValue != null){
            if( itemReturnAttributeGetter != null  && !returnObjects){
              valueSetter($scope, itemReturnAttributeGetter($scope.selectedValue));
            } else {
              valueSetter($scope, $scope.selectedValue);
            }
          } else {
            if(returnObjects){
              var returnObject = {}
              returnObject[itemReturnAttributeName] = $scope.searchString;
              valueSetter($scope, returnObject);
            } else {
              valueSetter($scope, $scope.searchString);
            }
          }
        };

        $scope.resultItemClicked = function(item){
          $scope.selectedValue = item;
          $scope.searchString = $scope.getItemDisplayString(item);
          valueWasSelected = true;
          $scope.showResults(false);
        };

        $scope.filterItems = function(){
          var searchObject = {};
          if( itemAttributeName != null ){
            searchObject[itemAttributeName] = $scope.searchString;
          } else {
            searchObject = $scope.searchString;
          }

          var tempList = $filter('filter')($scope.items, $scope.searchString);
          $scope.filteredItems = tempList;
          if( $scope.searchString.length > 0 && !$scope.resultsVisible.value )
            $scope.showResults(true);
          if( $scope.searchString.length == 0 && $scope.resultsVisible.value )
            $scope.showResults(false);
        };

        $scope.$watch('searchString', function(){
          $scope.filterItems();
          if( !valueWasSelected )
            $scope.selectedValue = null;
          else
            valueWasSelected = false;
          $scope.updateSourceValue();
        }, true);
      }
    }
  }
])
\$\endgroup\$
4
  • \$\begingroup\$ Anything against Angular's native select directive: docs.angularjs.org/api/ng/directive/select ? \$\endgroup\$
    – Dmitri Zaitsev
    Commented Apr 8, 2015 at 12:02
  • \$\begingroup\$ There is no ability to enter arbitrary values, nor is there the ability to search through the list as far as I know. My goal is to provide both of these options. \$\endgroup\$
    – fretsejaz
    Commented Apr 9, 2015 at 18:02
  • \$\begingroup\$ I see. But you could still reuse native directives and build just what is missing on top of them. That would shorten your code. \$\endgroup\$
    – Dmitri Zaitsev
    Commented Apr 10, 2015 at 12:32
  • \$\begingroup\$ The angular native select is an actual html select box. I'm pretty sure trying to redefine it's behavior would cause more complexity. Though I've never tried. \$\endgroup\$
    – fretsejaz
    Commented Apr 11, 2015 at 15:16

1 Answer 1

Reset to default
3
\$\begingroup\$

Having looked at your code I can see one major problem - your directive hides but does not isolate its scope.

That makes it vulnerable to both external and internal changes. Internally you may change a variable and forget to adjust it and suddenly your directive is using the same named variable from outside! Imagine all the bad things can happen to your directive!

That makes the maintenance a nightmare - you can't even use reliable tests to catch the "leaky" variables that you forgot to declare. They will keep "sleeping" quietly deep inside your code until the wake-up explosion :)

Your directive seems to be intended as encapsulated drop-in component. For which Angular's isolate scope is the best solution.

The isolated scope: {...} declaration inside your directive tells you exactly which attributes are "allowed" inside and how are they named. You see it clearly right at the top of your directive, which makes your code more readable and maintainable. Now you can change the outside-inside coupling nicely inside that declaration. That also would make your code cleaner, shorter and DRYer.

As side remark, I would generally try to avoid, if possible, the low-level "system functions" such as $parse, $filter etc. They are used by Angular internally to expose more user-friendly (and reader-friendly) API to you. Such as the isolated scope binding and using filters rather inside your templates (i.e. what it is meant to).

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for looking at this. I recently fixed the scope issue, and these fixes are in the github repo (which has a demo now!). Unfortunately I don't how to implement this without parse as I need to get ahold of the source list. \$\endgroup\$
    – fretsejaz
    Commented Apr 11, 2015 at 15:15
  • \$\begingroup\$ @fretsejaz You are using $parse for 2-way binding which is natively supported by the isolated scope. If you need more help, make another post here. \$\endgroup\$
    – Dmitri Zaitsev
    Commented Apr 11, 2015 at 15:23
  • \$\begingroup\$ So instead use something like scope["attributeName"] instead of the getter/setter functions? \$\endgroup\$
    – fretsejaz
    Commented Apr 11, 2015 at 17:32
  • \$\begingroup\$ @fretsejaz No, just use the 2-binding like scope: {a : "="}. \$\endgroup\$
    – Dmitri Zaitsev
    Commented Apr 12, 2015 at 2: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.