4
\$\begingroup\$

To get an idea of what the controller code reflects, here is a screenshot:

enter image description here

Basically, the idea is that you print out a list of football players which you can then filter by their name, price, field position, and team. I'm not sure whether it's better to handle all the scope logic within a single controller, or break it down into separate controllers, isolating the filters from the player list, pagination, and so on. I'm currently going with the single controller approach, and here's what I have:

app.controller('playersController', function($scope, $filter, minMax, buildOptions, PlayersModel, TeamsModel, CommonValues){
    $scope.basehref         = CommonValues.getBaseHref();
    $scope.imgbase          = CommonValues.getImgBase();
    $scope.players          = PlayersModel;
    $scope.teams            = TeamsModel;
    $scope.predicate        = 'team.name';
    $scope.minMax           = minMax($scope.players);

    $scope.filters          = { // filter model 
        searchString: '',       // string input from user
        selectedTeam: {},       // selected team
        selectedTeamMs: [],     // selected teams from multiselect
        roles: {
            1: true,            // forward
            2: true,            // defense
            3: true,            // midfield
            4: true             // goalie 
        },
        slider: {               // set up boundaries for slider filter
            min: $scope.minMax[0],
            max: $scope.minMax[1],
            step: 10000
        },
        pagination: {           // set up pagination
            totalItems: $scope.players.length,
            currentPage: 1,
            maxSize: 5,
            pages: buildPages()
        }
    };
    var cache = $scope.players;   // save original player model for later use


    /**********
     * Watchers
     **********/

    // watch for filter model changes, start filtering
    $scope.$watch('filters', function(newVal, oldVal){ 
        if (newVal !== oldVal || newVal.id !== oldVal.id || newVal.length !== oldVal.length) {
            var result;

            result = $filter('textFilter')(cache, $scope.filters);                 // filter by phrase
            result = $filter('roleFilter')(result, $scope.filters);                // filter by field position
            result = $filter('teamFilter')(result, $scope.filters.selectedTeamMs); // filter by team

            $scope.players = result;
        }
    }, true);


    /***********
     * Scope API
     ***********/

    // pagination logic
    $scope.changePage  = function(newVal, oldVal) { 
        var maxSize    = $scope.filters.pagination.maxSize;
        var offset     = maxSize * oldVal;
        var result     = [];

        if (newVal < oldVal)  offset = (maxSize * newVal) - maxSize;

        for (var i = 0; i < cache.length; i++) {
            if (i >= offset) result.push(cache[i]);
        }  
        $scope.players = result;
    };


    /****************
     * Helper methods
     ****************/
    function buildPages() {
        return null;
    }
});

Is it ok to encapsulate all the logic into that one controller, including helper methods, and so on, or should I refactor?

\$\endgroup\$
2
\$\begingroup\$

First of all, for a better user experience, I would avoid pagination in favour of infinite scroll. Here is a simple Angular infinite scroll implementation

I presume you are aware of minification-proof technique .controller('name', ['$scope', function($scope){}], so won't go into that.

Next, I see many fields like $scope.players, $scope.teams, ..., mixed with $scope.filters, which can lead to namespace conflicts as the app grows. Hence I would gather all data-related fields into one object like $scope.data. Then you can simply pass the whole object from your service instead of assigning one-by-one.

The hard-coded declation $scope.predicate = 'team.name'; is mixed with others provided by the service. Perhaps it belongs to a Config service together with other hard-coded properties.

Also I presume you are aware of the difference between using $scope.field vs $scope.field.subfield in terms of their interactions with parent and child scopes.

EDIT. Unless you know what you are doing, a recommended practice is to use $scope.field.subfield. That way, in a child controller, you have both read and write access to it. If instead you use $scope.field, then changing it from a child controller will add a variable on the local scope but won't change the parent scope as you likely intend, see here.

EDIT. See also Misko's video on best practices, including putting the dot inside your $scope property.

minMax($scope.players); suggests that you are using service minMax just for one function. I would gather those functions into one bigger service like Utils. You already have lots of dependencies for your controller, so lowering that number can be a good thing. Similar for $scope.teams = TeamsModel;.

I am not sure why you define filters inside a controller, rather than with .filter as common.

Usually filters declared via .filter update themselves, so no need for watchers.

\$\endgroup\$
10
  • \$\begingroup\$ 1. How can $scope.xy lead to namespace conflicts, when the scope is bound to the current controller? 2. Why encapsulate all data into one service? I'll need each service individually in other controllers, and passing around both as one seems like a waste. 3. How can those filters be defined, when the code correctly executes filters that are passed with the $filter provider and are defined elswhere? 4. How can a filter update itself, when I don't watch for either model changes in the controller, or in the template? \$\endgroup\$ – Metzger Apr 19 '14 at 9:02
  • 1
    \$\begingroup\$ If performance is an issue, you can only keep the visible part and slightly above and below inside the DOM. I find infinite scrolling even more convenient on a device, where scroll is so much easier than navigating the tiny page buttons. \$\endgroup\$ – Dmitri Zaitsev Apr 19 '14 at 10:34
  • 1
    \$\begingroup\$ Here is also great explanation why not to use primitives on $scope: nathanleclaire.com/blog/2014/04/19/… \$\endgroup\$ – Dmitri Zaitsev Apr 22 '14 at 13:19
  • 1
    \$\begingroup\$ Yes, everything except directly glueing your models to views is generally advised to take out of controller. All configs, filters, helpers... Also you can use angular.equals to compare objects in your watcher (but then again, using Angular filters would remove the need for the watcher here in first place). \$\endgroup\$ – Dmitri Zaitsev Apr 22 '14 at 16:44
  • 1
    \$\begingroup\$ Exactly, here is a great article on precisely that: blog.safaribooksonline.com/2014/04/08/…. Your data services provide the API to manipulate the data, used from controller. Controller can send raw data to the model service like field.addPlayer(player), which can in turn update the player. \$\endgroup\$ – Dmitri Zaitsev Apr 22 '14 at 17:08

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.