0
\$\begingroup\$

I'm very new to AngularJS and just started in a new project which uses AngularJS for Web UI. I was looking into our code and found out that we have huge functions in controllers. Like, one 100-LOC function which does a lot. Logically, our Sonar complains about it and for me as a novice it's quite difficult to understand what's going on.

A short summary of what the code does:

  • The code calls the backend REST interfaces to get a timetable of the train.
  • This timetable (basically a sequence of departures/arrivals to/from stop) is later on displayed in the UI.
  • The retrieved timetable is used to prepare objects later on used by UI for the rendering.

I thought it would be good to structure these big functions in controllers in smaller functions. I'd like to have several smaller easier-to-understand functions, but I don't know what is the correct way to achieve this.

What I've found was this resource which had an example of a controller function divided into smaller functions (see the DropdownController). Here's my take of this pattern on one of our controllers:

myControllers.controller('TrainDetailsController', ['$scope','trainResource', 'trainSequenceResource', '$location', '$log',
    function($scope, trainResource, trainSequenceResource, $location, $log) {

        var self = this;

        $scope.filter = {};
        $scope.stops = null;
        $scope.host_location = $location.host();

        this.reloadTrain = function() {
            if($scope.filter.date && $scope.filter.number) {
                $scope.stops = [];
                self.queryTimetable();
                self.queryTrainSequence();
            }
        };

        this.queryTimetable = function() {
            trainResource.query({
                number: $scope.filter.number,
                day: $scope.filter.date.day,
                month: $scope.filter.date.month,
                year: $scope.filter.date.year
            }, function (timetable) {
                self.processTimetable(timetable);
            });
        };

        this.processTimetable = function (timetable) {
            $scope.train = timetable.train;
            $scope.classification = timetable.classification;

            if ($scope.stops.length !== 0) {
                $scope.stops = [];
            }

            var hasCoupledTrain = false;

            for (var i = 0; i <= timetable.sections.length; i++) {
                var firstSection = i === 0;
                var lastSection = i === timetable.sections.length;
                var arrivalSection = i > 0 ? timetable.sections[i - 1] : null;
                var departureSection = i < timetable.sections.length ? timetable.sections[i] : null;
                var stop = self.createStop(arrivalSection, departureSection, firstSection, lastSection);
                $scope.stops.push(stop);
                hasCoupledTrain = hasCoupledTrain || stop.hasCoupledTrain;
            }

            $scope.hasCoupledTrain = hasCoupledTrain;           
        };

        this.createStop = function (arrivalSection, departureSection, firstSection, lastSection) {
            var coupledTrain = departureSection ? departureSection.coupledTrain : undefined;
            var isCouplingStart = arrivalSection && arrivalSection.coupledTrain === undefined && coupledTrain !== undefined;
            var isCouplingEnd = departureSection && arrivalSection && arrivalSection.coupledTrain !== undefined && coupledTrain === undefined;
            var showCouplingLine = coupledTrain !== undefined;
            var showCouplingFinalDecoration = (showCouplingLine && firstSection) || (lastSection &&  arrivalSection.coupledTrain !== undefined);
            var showCouplingStartDecoration = isCouplingStart;
            var showCouplingEndDecoration = isCouplingEnd;

            if (isCouplingEnd) {
                coupledTrain = arrivalSection.coupledTrain;
            }

            return {
                station: arrivalSection ? arrivalSection.arrival.station : departureSection.departure.station,
                platform: arrivalSection ? arrivalSection.arrival.platform : departureSection.departure.platform,
                //TODO: put planned platform here
                platformPlanned: null,
                arrivalTime: arrivalSection && arrivalSection.arrival ? arrivalSection.arrival.time : null,
                departureTime: departureSection && departureSection.departure ? departureSection.departure.time : null,
                departureSection: departureSection,
                arrivalSection: arrivalSection,
                status: arrivalSection ? arrivalSection.arrival.status : departureSection.departure.status,
                coupledTrain: isCouplingStart || isCouplingEnd ? coupledTrain : undefined,
                hasCoupledTrain : coupledTrain !== undefined,                
                showCouplingLine: showCouplingLine,
                showCouplingFinalDecoration: showCouplingFinalDecoration,
                showCouplingStartDecoration: showCouplingStartDecoration,
                showCouplingEndDecoration: showCouplingEndDecoration
            };
        };

        this.queryTrainSequence = function () {
            trainSequenceResource.query({
                    year: $scope.filter.date.year,
                    month: $scope.filter.date.month,
                    day: $scope.filter.date.day,
                    number: $scope.filter.number
                },
                function (trainSequence) {
                    self.processTrainSequence(trainSequence);
                }
            );
        };

        this.processTrainSequence = function(trainSequence) {
            $scope.trainSequence = trainSequence;
        };

        $scope.$watch('filter.date', function() {
            $log.info("Date changed. Reload Train.");
            self.reloadTrain();
        });

        $scope.$watch('filter.number', function() {
            self.reloadTrain();
        });

    }]);

I'd be grateful for a review. My main objective is to find out if it is the right way to structure controller functions. Should I move more code to the services? Is self and this usage OK? Is it OK to assign functions to this properties?

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Taking the example from the link you sent, which is good, you see the controller is doing not much, in their words 'Normal controller stuff!'

The controller on the link provided is either watching ($scope.$on, $scope.$watch), doing an initialization (init) or some really small logic (init, toggle, isOpen)

You should aim for the same in your controller. Either watch a variable/event, or do some minimal logic.

For anything that is not a watch, use self instead of $scope, as in the example. (Good article to that extent)

Supposing trainResouce and trainSequenceResource are services, you could call them directly on the view (HTML), even passing only $scope.filter and accessing the attributes inside the service method

The function to create a stop can definitely be extracted to a directive or service

Finally, for the processTimetable function, you could try the angular.forEach syntax.

Once you've done all that changes, you'll notice there is still big pieces of code on the bigger functions, which you can later try and divide in smaller ones.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.