2
\$\begingroup\$

I made a simple AngularJS text function. The purpose is to provide an array of content to display. A 'slider' will be made with next and previous buttons that hide on the last and first slide, respectively. I can't help but feel it isn't very efficient. Any thoughts?

Here is a JSFiddle for those who want a live example.

Here is the code:

<script>
    var app = angular.module('app', []);
    app.controller('ctrl', ['$scope',

    function($scope) {
        $scope.boxes = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'];
        $scope.currentIndex = 0;

        $scope.$watch('currentIndex', function(currentIndex) {
            $scope.active = $scope.boxes[$scope.currentIndex];
            if (currentIndex === 0) {
                $scope.first = true;
            } else if (currentIndex == $scope.boxes.length - 1) {
                $scope.last = true;
            } else {
                $scope.first = false;
                $scope.last = false;
            }
        });
    }]);
</script>
<div ng-app="app" ng-controller="ctrl">
    <div style="width:50px;height:50px;background:yellow;" ng-click="currentIndex = currentIndex-1" ng-show="!first">PREVIOUS</div>
    <div style="width:100px;height:100px;background:red;" ng-repeat="box in boxes" ng-show="box == active">{{box}}</div>
    <div style="width:50px;height:50px;background:blue;" ng-click="currentIndex = currentIndex+1" ng-show="!last">NEXT</div>
</div>
\$\endgroup\$
1
\$\begingroup\$

Instead of setting the active property to an element in the array, set it to the index of the element.

The reason for this being that almost all your operations are on the index and not the element itself. You only use that once: when you display the element. You can then eliminate your calls to indexOf because you already have the result. You can also simply the logic around first and last by centralizing it.

var app = angular.module('app', []);
app.controller('ctrl', ['$scope',
    function ($scope) {
        $scope.hello = 'hello world';
        $scope.boxes = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'];
        $scope.active = 0;
        $scope.move = function (x) {
            if(x) {
               //while incrementing keep the index in bounds using min
               $scope.active = Math.min($scope.active+1,$scope.boxes.length - 1);
            } else {
               //while decrementing keep the index in bounds using max
               $scope.active = Math.max($scope.active-1,0);
            }
        };
        $scope.$watch('active', function (active) {
            //it's usually possible for the first element to be the last as well
            //so unless you know you'll never have an array with 1 element
            //don't use an if-else here
            $scope.first = active == 0;
            $scope.last = active == $scope.boxes.length - 1;
        });
    }
]);
\$\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.