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?

share|improve this question

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.

share|improve this answer
    
Also, it's my hunch that the statements like vm.error = error; that are calls to functions aren't needed unless the view has something that uses them, such as ng-submit. Internal calls can be made to the functions directly. – cst1992 Mar 19 at 8:10

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.

share|improve this answer
    
Good suggestion. – cst1992 Aug 13 at 13:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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