Tukukkk

I'm somewhat new to JavaScript and AngularJS, and read in Doug Crockford's book that the hoisting function declaration can lead to issues. Any pointers on how to structure my file would be much appreciated.

(function() {

    'use strict';



    function PlayerProfileStatsCtrl($q, ColumnService, LookupService, PlayerService, PlayerStatsService) {

       var vm = this;



        var loadStats = function (player) {

            $q.all({

                catchingStats: PlayerStatsService.getCatchingStats(player.playerId),

                fieldingStats: PlayerStatsService.getFieldingStats(player.playerId),

                hittingStats: PlayerStatsService.getHittingStats(player.playerId),

                pitchingStats: PlayerStatsService.getPitchingStats(player.playerId),

                playerStats: LookupService.getByKeyPromise('lkplayerstat'),

                statTypes: LookupService.getListPromise('lkstattype')

            }).then(function (results) {



                // Associate stat result lists with string keyed stat types.

                vm.statResults = {

                    C: results.catchingStats.statList,

                    F: results.fieldingStats.statList,

                    H: results.hittingStats.statList,

                    P: results.pitchingStats.statList

                };



                // Assign the player stat lookup object.

                vm.playerStats = results.playerStats;



                // Assign the stat type lookup list.

                vm.statTypes = results.statTypes; 



                initStatColumnDefs();

            });

        };



        // Call loadStats() after the player promise is resolved.

        vm.player = PlayerService.getPlayer(loadStats);



        var initStatColumnDefs = function () {



            // The column definitions are contained in lists and associated by stat type.

            vm.statColumnDefs = {};



            for (var i = 0; i < vm.statTypes.length; i++) {



                // Get the current stat type key: { 'C', 'F', 'H', 'P' }

               var statTypeKey = vm.statTypes[i].statTypeLk;



               // Get the list of column names from the user.

               var userSelectedStats = ColumnService.getColumns('STATS_' + statTypeKey);



               var getStatColumnDefs = function (userColumns) {

                   var columnDefs = ;



                    // Each stat section will start with these columns.

                    columnDefs.push({ name: 'Season', fullName: 'Season', property: 'statDb.year' });

                    columnDefs.push({ name: 'Team', fullName: 'Team', property: 'team.teamName' });

                    columnDefs.push({ name: 'Level', fullName: 'Level', property: 'level.shortDescription' });



                    // Fielding has an additional column in before custom columns are displayed.

                    if (statTypeKey === 'F') {

                        columnDefs.push({ name: 'Pos', fullName: 'Position', property: 'statDb.positionLk' });

                    }



                    // Construct a custom stat column definition.

                    var getCustomColumnDef = function (userColumn) {

                        var currentStat = vm.playerStats[userColumn];

                        var displayName = currentStat.shortDescription;

                        var onHoverName = currentStat.description;

                        var statProperty = 'statDb.' + currentStat.statProperty;



                        // Format the custom stat according to the player stat lookup table.

                        var getFormattedStat = function (val) {

                            if (val === undefined) {  // Return null if there is no value for that stat.

                                return; 

                            } else {

                                if (!isNaN(val)) {  // Only process numbers.

                                    val = String(val.toFixed(currentStat.precision));



                                    // Remove leading zero.

                                    if (currentStat.leadingZeroFlg && val.startsWith('0')) {

                                        val = val.substr(1, val.length-1);

                                    }



                                     // Add a % sign to the end of the stat.

                                    if (currentStat.percentFlg) {

                                        val += '%';

                                    }

                               }

                               return val;

                           }

                    }

                    return { name: displayName, fullName: onHoverName, property: statProperty, format: getFormattedStat };

                }



                // Add a definition for each of the custom columns.

                for (var i = 0; i < userColumns.length; i++) {

                    columnDefs.push(getCustomColumnDef(userColumns[i]));

                }

                return columnDefs;

            }



            // Store the list of defintions by stat type.

            vm.statColumnDefs[statTypeKey] = getStatColumnDefs(userSelectedStats);



        }

    }

}



 angular

 .module('players')

 .controller('PlayerProfileStatsCtrl', [ '$q', 'ColumnService', 'LookupService', 'PlayerService', 'PlayerStatsService', PlayerProfileStatsCtrl ])

})();

Bearing in mind that you claim not to maintain this code anymore¹, there are a few suggestions I would have about this code:

• 
  define functions and accept parameters- that should allow the nested functions to be moved out... You may be able to utilize Partially applied functions.


• 
  catch rejected promises The code calls $q.all() with 6 promises, yet has no catch() callback! If any one of those promises would fail, should the user be notified?


• 
  bind function scope instead of assigning vm Utilize Function.prototype.bind() to set the context of this inside callback functions - like the promise callbacks, initStatColumnDefs(), etc. Then there is no need to define the extra variable vm - just use this.


• 
  

  Get rid of useless else - There is no need to use the else in the block cited below. If the conditional of the if statement evaluates to true then the return will be reached. Otherwise the rest of the code can be executed normally.

  
  
  

  
  

  if (val === undefined) {  // Return null if there is no value for that stat.
  
       return; 
  
  } else {
  
  

  
  

  
  

¹_{Baseball player statistics controller}