AngularJS controller to fetch and display employee data from database

Question

This is the code for an AngularJS controller that I've written:

app.controller('employeeController', function (employeeService) {

    var Controller = this;

    Controller.employees = {};
    Controller.fetchedEmployee = {};

    Controller.populateTable = function (data) {

        Controller.employees = data;
    };


    Controller.error = function (errorResponse) {
        console.log("error function called.");
        console.log(errorResponse);
        // Request Errors
        if(errorResponse.status === 400) alert("Error: " + errorResponse.data.error + ": There was a problem with the data you submitted. \nPlease enter valid form data.");
        else if(errorResponse.status === 401) alert("Error: " + errorResponse.data.error + ": You need to provide proper authorization credentials to continue.");
        else if(errorResponse.status === 403) alert("Error: " + errorResponse.data.error + ": You are not allowed to perform this action.");
        else if(errorResponse.status === 404) alert("Error: " + errorResponse.data.error + ": The requested file or resource was not found.");
        else if(errorResponse.status === 408) alert("Error: " + errorResponse.data.error + ": The connection timed out. Please try again later.");
        else if(errorResponse.status === 410) alert("Error: " + errorResponse.data.error + ": The requested resource is no longer available.");
        else if(errorResponse.status === 418) alert("Error: " + errorResponse.data.error + ": I'm a teapot, not a coffeepot.");
        // Server Errors
        else if(errorResponse.status === 500) {
            if(errorResponse.data.exception === "java.sql.SQLException") alert("Error establishing connection with database. Please try again later.");
            else alert("Error: " + errorResponse.data.error + ": Generic Server Error.");
        }
    };

    Controller.deleteEmployee = function(id) {

        employeeService.getEmployee(id).then(function(data) {
            Controller.fetchedEmployee = data.empInfo;
            var reallyDelete = confirm("Do you really want to delete " + Controller.fetchedEmployee.employeeName + "?");
            console.log(reallyDelete);
            if(reallyDelete === true) employeeService.deleteEmployee(id);

        }, Controller.error);

    };

    // Call Service to List all Employees
    console.log("Service called to populate table.");
    employeeService.getAllEmployees().then(Controller.populateTable, Controller.error);
    Controller.populateTable();
});

Not that this code hasn't gone through improvements; initially it was full of this.something or even worse, $scope.something (scope soup), which made it difficult to understand and even resulted in some of the code being broken.

However, it still feels sloppy. It feels like all the functions and variables have been 'injected' randomly in the controller; like someone randomly putting playing cards in a neatly shuffled deck or randomly placing books in a bookshelf.

Could anyone help me devise a methodology to organize this code to have a definite structure?

Answer 1

You should create a json object for your errors. Key - error number, value - custom error message. Try to be DRY. Just imagine situation, when i18n will be required.

Answer 2

This code can indeed be made more readable.

Reference from this AngularJS style guide from John Papa: https://github.com/johnpapa/angular-styleguide

• Controller can be replaced with vm, which stands for View-Model. Entirely a matter of preference, but shorter name will be easier to read.

• Functions are all function expressions. This introduces a limitation on their ordering, as they're interdependent. If declarations are used instead, we can split declarations and remaining code apart, and order them alphabetically for far higher readability.

• The controller can also be made a function declaration and getters can be used, eliminating the need to assign angular.module to a variable app. This is for the same reason as in the previous point.

• The last Controller.populateTable() is unnecessary. If you move the console.log inside the function then you'll realize it's being called twice. Not necessary at all.

• This will also allow the populateTable function to be called directly, without the need for a separate variable.

With all these changes, the code looks like this:

angular.module('myApp')
    .controller('employeeController', employeeController);

function employeeController(employeeService) {

    var vm = this; // stands for view-model

    vm.deleteEmployee = deleteEmployee;
    vm.employees = {};
    vm.error = error;
    vm.fetchedEmployee = {};

    // Call Service to list all employees
    employeeService.getAllEmployees()
        .then(function(data) { vm.employees = data; }, vm.error);
    console.log("Service called to populate table.");    

    //// Function Declarations /////////////////////////////////////////////////

    function deleteEmployee(id) {

        employeeService.getEmployee(id)
            .then(function(data) {
                vm.fetchedEmployee = data.empInfo;
                var reallyDelete = confirm("Do you really want to delete " + vm.fetchedEmployee.employeeName + "?");
                console.log(reallyDelete);
                if(reallyDelete === true) employeeService.deleteEmployee(id);

        }, vm.error);    
    }

    function error(errorResponse) {
        console.log("error function called.");
        console.log(errorResponse);
        // Request Errors
        if(errorResponse.status === 400) alert("Error: " + errorResponse.data.error + ": There was a problem with the data you submitted. \nPlease enter valid form data.");
        else if(errorResponse.status === 401) alert("Error: " + errorResponse.data.error + ": You need to provide proper authorization credentials to continue.");
        else if(errorResponse.status === 403) alert("Error: " + errorResponse.data.error + ": You are not allowed to perform this action.");
        else if(errorResponse.status === 404) alert("Error: " + errorResponse.data.error + ": The requested file or resource was not found.");
        else if(errorResponse.status === 408) alert("Error: " + errorResponse.data.error + ": The connection timed out. Please try again later.");
        else if(errorResponse.status === 410) alert("Error: " + errorResponse.data.error + ": The requested resource is no longer available.");
        else if(errorResponse.status === 418) alert("Error: " + errorResponse.data.error + ": I'm a teapot, not a coffeepot.");
        // Server Errors
        else if(errorResponse.status === 500) {
            if(errorResponse.data.exception === "java.sql.SQLException") alert("Error establishing connection with database. Please try again later.");
            else alert("Error: " + errorResponse.data.error + ": Generic Server Error.");
        }
    }
}

If anyone has any further additions, be sure to comment below.

Answer 3

A year after my March 2016 answer, there are still more improvements than what have been written in my earlier answer.

• As @Kindzoku says, using an object for the error codes will be better.

• I didn't know this at the time, but the .then function returns not only the data, but also the entire response(recent implementations of .success() and .error() that I've seen call this function internally and return the sub-parts). So the function should return the data, not the response as the "data". Alternatively, the promise is resolved internally by a service and it returns another with only the data body.

• Controllers are meant to be thin and hold only data(i.e. models). The code for resolving the errors should be moved to a service, and a new promise should be returned that is resolved only if none of the errors have occurred, or reject with that particular error.

• A mistake with the earlier answer is that code with a separate function body should always be wrapped in an Immediately Invoked Function Expression(IIFE). This gives the function local scope, and makes sure the function is only tied to the particular module. The IIFE-included function is executed as soon as JavaScript encounters it.

• Controller should start with a capital letter as it's a constructor.

Code:

The main controller:

(function(){
    angular.module('myApp')
        .controller('EmployeeController', EmployeeController);

    function EmployeeController(employeeService, errorService) {

        var vm = this; // stands for view-model

        vm.employees = {};
        vm.fetchedEmployee = {};

        vm.deleteEmployee = deleteEmployee;
        vm.error = error;


        // Call Service to list all employees
        employeeService.getAllEmployees()
            .then(function(response) { vm.employees = response.data; }, vm.error);
        console.log("Service called to populate table.");    

        //// Function Declarations /////////////////////////////////////////////////

        function deleteEmployee(id) {

            employeeService.getEmployee(id)
                .then(function(data) {
                    vm.fetchedEmployee = data.empInfo;
                    var reallyDelete = confirm("Do you really want to delete " + vm.fetchedEmployee.employeeName + "?");
                    console.log(reallyDelete);
                    if(reallyDelete === true) employeeService.deleteEmployee(id);

            }, vm.error);    
        }

        function error(errorResponse) {
            console.log("error function called.");
            console.log(errorResponse);
            // alert error
            alert(errorService.getErrorMessage(errorResponse.status, errorResponse.errorCode));
        }
    }
})();

An additional error service:

(function(){
    angular.module('myApp')
        .service('errorService', errorService);

    function errorService() {

        var errors = [
            {
                status: 400,
                errorMessage: "There was a problem with the data you submitted. \nPlease enter valid form data."
            },
            {
                status: 401,
                errorMessage: "You need to provide proper authorization credentials to continue."
            },
            {
                status: 403,
                errorMessage: "You are not allowed to perform this action."
            },
            {
                status: 404,
                errorMessage: "The requested file or resource was not found."
            },
            {
                status: 408,
                errorMessage: "The connection timed out. Please try again later."
            },
            {
                status: 410,
                errorMessage: "The requested resource is no longer available."
            },
            {
                status: 418,
                errorMessage: "I'm a teapot, not a coffeepot."
            },
            {
                status: 500,
                errorMessage: "Internal Server Error."
            }
        ];

        this.getErrorMessage = function(status, errorCode) {
            for(var i = 0; i < errors.length; i++) {
                if(errors[i].status === status) {
                    return "Error: " + errorCode + ": " + errors[i].errorMessage;
                }
            }
        };
    }
})();

A modified employee service that returns a promise, to be resolved in the controller:

(function(){
    angular.module('myApp')
        .service('employeeService', employeeService);

    function employeeService($http, $q) {

        this.getEmployee = function (id) {

            var deferred = $q.defer();
            // HTTP call
            $http.get("myurl")
                .then(function(response) {

                    deferred.resolve(response.data); // return the data body from response

                }, function(response){

                    deferred.reject({                // return custom error object
                        status: response.status
                        errorCode: response.error
                    });
                });
            return deferred.promise;
        };

        // ... similar code for other methods ...
    }
})();