3
\$\begingroup\$

I have been an Angular developer for more than 5-6 months and mostly I learned with my experience. However, recently after submitting code to some competition, I got a feedback that AgularJS best practices were not followed. I need some help understanding irregularities in my code.

Controller

crossover.controller("rootController", ['$scope', '$rootScope', '$mdToast', '$mdMedia', '$mdDialog', '$location', '$http', function($scope, $rootScope, $mdToast, $mdMedia, $mdDialog, $location, $http){
    $rootScope.isLoggedIn = false;
    $rootScope.sessionId = null;
    $scope.showPopup = function(ev, template) {
        var useFullScreen = ($mdMedia('sm') || $mdMedia('xs'))  &&     $scope.customFullscreen;
        $mdDialog.show({
          controller: DialogController,
          templateUrl: template,
          parent: angular.element(document.body),
          targetEvent: ev,
          clickOutsideToClose:true,
          fullscreen: useFullScreen
        })
        .then(function(answer) {
            if(answer){
                console.log("Hello");
                $mdToast.show(
                  $mdToast.simple()
                    .textContent('Request Sent!!!')
                    .hideDelay(2000).position('top right')
                );
            }          
        }, function() {
            //user cancelled wanna handle?    
        });
        $scope.$watch(function() {
          return $mdMedia('xs') || $mdMedia('sm');
        }, function(wantsFullScreen) {
          $scope.customFullscreen = (wantsFullScreen === true);
        });
    };
    $scope.logout = function(){            
        $rootScope.isLoggedIn = false;
        $rootScope.sessionId = '';
        $location.path("/login");             
        //content type sent is text/json but they are not text/json actually
        //so angular giving error while accepting them.
//        $http.get("http://localhost:8080/logout?sessionid=" + $rootScope.sessionId, {parseJSON: false}).then(function(result){           
//            $rootScope.isLoggedIn = false;
//            $rootScope.sessionId = '';
//            $location.path("/login");     
//        });        
    }

}]);

crossover.controller("loginController", ['$scope', '$http', '$rootScope', '$location', function($scope, $http, $rootScope, $location){
    $scope.user = {
        'userName' : '',
        'password': ''
    };
    $scope.doLogin = function(){        
        $http.get("http://localhost:8080/login?username=" + $scope.user.userName + 
                 "&password=" + $scope.user.password)
            .then(function(res){                
                if(res.data.sessionId == null){
                    console.log("Invalid credentials");
                    alert("Please provide correct credentials");
                }else{
                    $rootScope.sessionId = res.data.sessionId;
                    $rootScope.isLoggedIn = true;                     
                    $location.path("/dashboard");                    
                }
            });
    }
}]);

crossover.controller("dashboardController", ['$scope', '$location', '$rootScope',  function($scope, $location, $rootScope){    
    if($rootScope.isLoggedIn != true){
        $location.path("/login");
    }
    $scope.charts = [{
       'what' : 'Sales Total per Sales Man',
       'where': 'salestotal/salesman'
    }, {
        'what' : 'Sales Total per Month',
        'where': 'salestotal/month'
    },{
        'what': 'Top 5 Sales Order',
        'where': 'top5/order'
    },{
        'what': 'Top 5 Sales Men',
        'where': 'top5/salesman'
    }];
    $scope.goto = function(str){
        $location.path(str);
    }
}]);

crossover.controller("pieController", ['$scope', '$rootScope', '$http', '$location', function($scope, $rootScope, $http, $location){
    if($rootScope.isLoggedIn != true){
        $location.path("/login");
    }    
    $scope.labels = [];
    $scope.data = [];

    $scope.refresh = function(){
        $http.get('http://localhost:8080/salesmandata?sessionid=' + $rootScope.sessionId).success(function(result){            
            $scope.labels = [];
            $scope.data = [];
            angular.forEach(result.data, function(obj){                          
                $scope.labels.push(obj[0]);
                $scope.data.push(obj[1]);               
            });
        }); 
    }
    $scope.refresh();
}]);

crossover.controller('barController', ['$scope', '$rootScope', '$http', '$location', function($scope, $rootScope, $http, $location){
    if($rootScope.isLoggedIn != true){
        $location.path("/login");
    }    
    $scope.labels = [];
    $scope.data = [[]];
    $scope.series = ['Series A'];    
    $scope.options = { scaleShowVerticalLines: false};    
    $scope.refresh = function(){
        $http.get('http://localhost:8080/lastyeardata?sessionid=' + $rootScope.sessionId).success(function(result){            
            $scope.labels = [];
            $scope.data = [[]];
            angular.forEach(result.data, function(obj){                          
                $scope.labels.push(obj[0]);
                $scope.data[0].push(obj[1]);               
            });
        }); 
    }
    $scope.refresh();        

}]);

crossover.controller('part3Controller', ['$scope', '$rootScope', '$http', '$location', function($scope, $rootScope, $http, $location){
    if($rootScope.isLoggedIn != true){
        $location.path("/login");
    }    
    $scope.items = [];
    $scope.refresh = function(){
        $http.get('http://localhost:8080/topsalesorders?sessionid=' + $rootScope.sessionId).success(function(result){            
            $scope.items = [];                
            angular.forEach(result.data, function(obj){
                $scope.items.push(obj);
            });
        }); 
    }
    $scope.refresh();        
}]);

crossover.controller('part4Controller', ['$scope', '$rootScope', '$http', '$location', function($scope, $rootScope, $http, $location){
    if($rootScope.isLoggedIn != true){
        $location.path("/login");
    }    
    $scope.items = [];
    $scope.refresh = function(){
        $http.get('http://localhost:8080/topsalesmen?sessionid=' + $rootScope.sessionId).success(function(result){            
            $scope.items = [];            
            angular.forEach(result.data, function(obj){
               $scope.items.push({
                   'Name': obj[0],
                   'value': obj[1]
               });
            });
        }).error(function(result){
            alert("some error occurred");
        }); 
    }
    $scope.refresh();            
}]);

function DialogController($scope, $mdDialog) {
  $scope.hide = function() {
    $mdDialog.hide();
  };
  $scope.cancel = function() {
    $mdDialog.cancel();
  };
  $scope.send = function(answer) {
    $mdDialog.hide(answer);
  };
}

I have shared the rest of the code on Google Drive.

\$\endgroup\$
  • 1
    \$\begingroup\$ Straight away, you're using angular.element in a controller. That alone is a big red flag to me - DOM access never goes in the controller. \$\endgroup\$ – Dan Jan 12 '16 at 12:55
  • \$\begingroup\$ @DanPantry that is standard code from demos on material ui site \$\endgroup\$ – codeomnitrix Jan 12 '16 at 12:59
  • 1
    \$\begingroup\$ then the material ui site is wrong. Do not manipulate the DOM in controllers. \$\endgroup\$ – Dan Jan 12 '16 at 12:59
  • \$\begingroup\$ Ok I will make a note of that...thanks for pointing that out...Please point out others...Thanks \$\endgroup\$ – codeomnitrix Jan 12 '16 at 13:07
1
\$\begingroup\$

I'd suggest reading the John Papa style guide as it's endorsed by the angular team.

Typically - if your controller doesn't have to $watch or $broadcast or do event stuff then you shouldn't really be using $scope. Instead use:

var vm = this;
vm.variableA = 'Some value';

vm.getStuff = getStuff;
function getStuff() {...}

In a directive definition this can be referred to using controllerAs

controller: 'MyController',
controllerAs: 'vm'

Any functions/variables are then referred to as vm.whateverItsCalled in the template.

You could combine the two above to this:

controller: 'MyController as vm'

Also - did you submit any tests with your code to prove that it works?

| improve this answer | |
\$\endgroup\$

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.