updateBuildChart method with a lot of dependencies

Question

I've some refactoring issue with one method. This method has a lot of dependencies

 function updateBuildChart(Filter, PagerFactory, ChartBuilder, LimitsManager) {
     Filter.clearFilters();

     this.currentReports = $scope.$parent.reports;

     $scope.pager = PagerFactory.Create({
         items: this.currentReports
     });

     this.originalReports = angular.copy(this.currentReports);
     LimitsManager.calcFteLimits();

     if (this.currentReports.length > 0) {
         $scope.showGraph = true;
         ChartBuilder.buildChart(this.currentReports);
     } else {
         $scope.showGraph = false;
     }

     Filter.setReports(self.currentReports);
 };

Now I want to split this big method to a small parts. Do you have some ideas how to change it?

Answer 1

The function is named updateBuildChart, but building a chart seems a very tiny part of all its responsibilities. If we kept only what its name implies and removed everything else:

function updateBuildChart(ChartBuilder) {
    var currentReports = $scope.$parent.reports;
    if (currentReports.length > 0) {
        ChartBuilder.buildChart(currentReports);
    }
}

However, without seeing all your code, it's hard to advise. Mainly, it's not clear if the statements in the function have side effects or not. So I'm just going to go over line by line and ask you some questions that might help you clean up this thing.

---

The function starts with using Filter and ends with using Filter, and it doesn't use it anywhere in between:

Filter.clearFilters();

// ...

Filter.setReports(self.currentReports);

Does this ordering matter? Will your program still work if you move the Filter.setReports call right after Filter.clearFilters? If not, then these calls will be good to group together.

It will be even better if you move them to a dedicated function whose responsibility is to clear filters and set the reports, perhaps something like resetFilters(Filter).

And move the call to this new function outside of updateBuildChart, where it really doesn't belong.

---

What is this for?

this.currentReports = $scope.$parent.reports;

Is it just to be a shortcut for $scope.$parent.reports in the rest of the function, or will this have a side effect outside this function? If it's the first case, then it would be better to use var currentReports = instead.

---

PagerFactory is only referenced in one place:

$scope.pager = PagerFactory.Create({
    items: this.currentReports
});

Similar to the treatment of Filter earlier, it would be better to move this into a dedicated function, say updatePager, and call it outside of this function.

---

What is this for?

this.originalReports = angular.copy(this.currentReports);

Since this.originalReports is not referenced again within the function, it would seem that this produces a side effect outside the function. Or else, it's pointless and should be deleted.

---

The only references to LimitsManager:

LimitsManager.calcFteLimits();

What is it doing there in the middle of the function? Does it need to be in the middle? Can it be at the start or at the end? Or does it have a side effect that makes a difference for the code that appears before or after it?

---

If the code dealing with ChartBuilder is moved out to its own method, then this part can be simplified:

if (this.currentReports.length > 0) {
    $scope.showGraph = true;
    ChartBuilder.buildChart(this.currentReports);
} else {
    $scope.showGraph = false;
}

To this:

$scope.showGraph = this.currentReports.length > 0;

---

If I make the optimistic and possibly completely false assumption that the statements in the function produce no side effects, I would replace it with this code:

function updateCharts(ChartBuilder) {
    var currentReports = $scope.$parent.reports;
    if (currentReports.length > 0) {
        ChartBuilder.buildChart(currentReports);
        $scope.showGraph = true;
    } else {
        $scope.showGraph = false;
    }
}

function updateFilters(Filter) {
    Filter.clearFilters();
    Filter.setReports($scope.$parent.reports);
}

function updatePager(PagerFactory) {
    $scope.pager = PagerFactory.Create({
        items: this.currentReports
    });
}

function updateLimits(LimitManager) {
    LimitsManager.calcFteLimits();
}

Once again, this decomposition is only useful if these functions really don't have side effects, which only you can tell. If they have side effects, then the order in which you call them might matter. In that case, you should at least document it. A better solution would be to reorganize the code to convert the side effect to direct effect, visible in the code.