7
\$\begingroup\$

I have a directive called meetingList with its own isolated scope that receives a list of meetings and displays the filtered items based on some criteria. The controller for this directive holds two variables the meetingList and the filteredMeetingList. The scope where this directive is created needs to know how many items the directive is displaying (filteredMeetingList.length) in order to do something (outside the directive). I am currently exposing this variable using two way databinding on the isolated scope to send the variable outside.

Is there a better way of doing this? is there some kind of standard for this?

I set up a simple Plunker to illustrate the scenario.

HTML

  <body ng-app="test">
    <div ng-controller="AppCtrl as appCtrl">
      <meeting-list meeting-list="appCtrl.meetingList" filtered-meetings-count="appCtr.filteredMeetingsCount"></meeting-list>
      Total meetings {{appCtrl.meetingList.length}} <br>
      Meetings being displayed by the meetingList directive {{appCtr.filteredMeetingsCount}}
    </div>
  </body>

Javascript

  var app = angular.module('test', []);

  app.directive('meetingList', function(){
    return{
      restrict: 'E',
      scope: {
        meetingList: '=',
        //this is the variable that I use to expose the length of the filtered meetings to the external scope
        filteredMeetingsCount: '='
      },
      controller: function($scope){
        var self = this;
        var meetingList = $scope.meetingList;
        var filteredMeetingList = $scope.filteredMeetingList = [];

        filterMeetings();
        //Here I set the value after filtering
        $scope.filteredMeetingsCount = filteredMeetingList.length;
        function filterMeetings()
        {
          filteredMeetingList.push(meetingList[1]);
          filteredMeetingList.push(meetingList[2]);
        }

      },

      template: '<div><span ng-repeat="meeting in filteredMeetingList"> {{meeting}} </span></div>'
    }
  });

  app.controller('AppCtrl', function($scope){
    var self = this;
    self.meetingList = ["meeting1", "meeting2", "meeting3"];
  });
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Did you know you can embed an executable code snippet in your post? Try to edit your post, and Ctrl+M to insert a Stack Snippet - no need for 3rd-party links! :-) \$\endgroup\$
    – Mathieu Guindon
    Commented Oct 22, 2015 at 18:10
  • \$\begingroup\$ Angular Style Guide github.com/johnpapa/angular-styleguide ... hope it helps \$\endgroup\$
    – rave
    Commented Nov 3, 2015 at 23:48

3 Answers 3

Reset to default
1
\$\begingroup\$

Given your code, it only filters the list and assigns the value on initialization. From the outside world, the count appears like a read-only value. This is ok... sort of.

One problem I see is that the count can be bound somewhere else and potentially manipulated. This potentially makes the count out of sync with the list count. Your list could say one thing, the count another. I don't know of any way to make it read-only. You could provide a function into the directive, sort of an onchange, where it gets called on modification in the directive and passes in the value there. This would mean that the controller holds responsibility of caching that value in a property. Just make sure that property also doesn't get manipulated though.

Another way you could improve this and avoid the 2-way binding "hack" is to move out the filtering to the enclosing controller. That way, the controller knows about the filtered list and it's length. All you have to do then is to provide that filtered list into your directive, since all it does is just render that list.

An even better way (I guess) is to move out the list and logic into a service/factory. That way, your "single source of truth" is a service, and not the controller nor directive. All you have to do then is pull in the data (full list and a filtered list) from the service/factory into the controller (controller will know about both), and pass in the filtered list to the directive. Any operations that manipulate the data lives in the service/factory, and done via function calls to it. It's essentially what React does with it's Flux architecture, isolating state and state changes into one location to prevent confusion caused by binding and multiple actors.

\$\endgroup\$
1
\$\begingroup\$

Please note I'm fairly new to Angular but we try to keep to best practices and use them.

The things I did:

  • Cleared the syntax a bit. This is mostly to fit conventions I'm used to, but please keep in mind that keeping consistent spacing and braces placement can be vary useful. Linters such as ESLint can help you with that (and much more).

  • Aliased controller context with 'vm' for a number of reasons.

  • Moved filtering logic to a service (de facto a factory, which doesn't call construction logic and is a bit more flexible - read more). This will cover a situation when we'll need more complex logic for filtering, that will be more maintainable and reusable.

    app.controller('AppCtrl', function(MeetingFilteringService){
    var vm = this;
    vm.meetingList = ["meeting1", "meeting2", "meeting3"];
    vm.filteredMeetings =   MeetingFilteringService.filterMeetings(vm.meetingList);


    app.factory('MeetingFilteringService', function() {
    return {
        filterMeetings: filterMeetings
    }

    function filterMeetings(meetingList) {
        var result = [];

        // here you can set up more detailed criteria
        result.push(meetingList[1]);
        result.push(meetingList[2]);

        return result;
    }
});

  • As Joseph suggested, moved filtering to controller - this is probably controller stuff, to care what goes the the display component.

    app.controller('AppCtrl', function(MeetingFilteringService){
    var vm = this;
    vm.meetingList = ["meeting1", "meeting2", "meeting3"];
    vm.filteredMeetings = MeetingFilteringService.filterMeetings(vm.meetingList);
});

  • Directive is now stripped of logic, but I remade it so it uses controller as syntax and bound-to-controller bindings, instead of using scope. This is widely regarded as a good practice in dealing with directives. Note that it's only a template to show how to handle situation if more complex logic would go in the directive.

    app.directive('meetingList', function(MeetingFilteringService) {
    return {
        (...)
        bindToController: true,
        controllerAs: 'meetings',
        (...)

       template: '<div><span ng-repeat="meeting in meetings.meetingList"> {{meeting}} </span></div>'

  • The controller only passes the already filtered list to the directive itself:

    <meeting-list meeting-list="appCtrl.filteredMeetings"></meeting-list>

Here's the full plunkr code

\$\endgroup\$
1
  • \$\begingroup\$ You shouldn't expect long comments about how to improve your answer, as comments are not for discussions! Though, you've written a very good first answer! The only thing I'd change is to bring back the information from the link about the aliased controller context. Links can die! :) \$\endgroup\$
    – IEatBagels
    Commented Oct 23, 2015 at 14:00
0
\$\begingroup\$

First I wouldn't name the directive and an attribute the same meeting-list. The attribute can be better named like meeting-list-source or meetings-source.

Next, appCtr.filteredMeetingsCount is not defined on your controller. This is counter to the basic MVC structure where View reads from the Model, which is here the ViewModel defined by Angular controller. It works but makes the code fragile and harder to read. Ideally the view should not write into Controller, but rather use commands defined by the controller to update the model.

I don't see your filter and its purpose. But normally you would either create a dedicated Angular Filter and put it into the view, or let a dedicated service do the filtering and the counting and anything else. There are several responsibilities here and it is better practice to separate them, rather than put all the logic into one directive.

\$\endgroup\$

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.