HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Controllers for interacting with a vacation service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withvacationcontrollersinteractingservicefor

Problem

I have a controller called VacationController; in this controller, I retrieve a list of vacations which will be displayed in a grid.

In the grid, I have the option to create a new vacation through a popup window. Should I create a new controller for this popup, considering that the model is represented by one row of the vacation list?

Those are my controllers:

app.controller('app.VacationController', ['$scope', 'vacationService', function ($scope, vacationService) {

    var vm = this;

    vm.model = {
        hasError: false,
        errorMessage: '',
        hasData:false,
        data: []
    }

    vacationService.get().success(function (vacations) {
        vm.model.hasData = vacations.length !== 0;
        vm.model.data = vacations;
    }).error(function() {
        vm.model.hasError = true;
        vm.model.errorMessage = 'Error';
    });

}]);

app.controller('app.NewVacationController', ['$scope', 'vacationService', function ($scope, vacationService) {

    var vm = this;
    vm.hasError = false;
    vm.error = '';
    vm.data = null;

    vacationService.getById(0).success(function (vacation) {
        vm.data = vacation;
    });

    vm.createNewVacation = function (viewModel) {
        vacationService.post(viewModel.data).success(function () {
            alert('success');
        });
    }
}]);

Solution

Should I create a new Controller for Insert?

Yes, and here's why. From a maintenance perspective, this is nightmare waiting to happen. The problem will start to appear when you want your popup used somewhere else independent of the list. However, if you bound your popup to the list controller, you can't move it out because you potentially bound the UI of the popup to some variable in the list controller.

The state of the popup should be independent of the list, thus you need to have a controller (or should I call it "view-model") for it.

As for your code:

-
I suggest you use the more standard then for promises instead of success and error. This makes it easier to swap out your promise libs/native promises when the time comes.

-
Use breakpoints to debug code instead of alert (or console or debugger). This prevents you from potentially leaving behind debugging code when you push to production.

Context

StackExchange Code Review Q#110247, answer score: 3

Revisions (0)

No revisions yet.