Controller structure for a browser game module with filters and pagination

Question

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

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?

Answer 1

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.