3
\$\begingroup\$

I am an Angular.JS beginner and this is my first application. I hope you give me some advices what to do to have better, optimized code.

This application manages the data from database, shows it in table, allows to add new rows, edit existing records and delete them.

This is how the application looks

Here is a content of my controller. I have only one controller, and one module file. I don't know whether I should split it in a few other controllers (it takes 215 lines).

(function() {
    'use strict';

    angular
        .module('app')
        .controller('ManageController', ManageController);

    function ManageController($http) {
        var vm = this;

        vm.addShirt = addShirt;
        vm.cancelForm = cancelForm;
        vm.checkCmdField = checkCmdField;
        vm.checkTeamField = checkTeamField;
        vm.keypress = keypress;
        vm.removeShirt = removeShirt;
        vm.submitAction = submitAction;
        vm.shirts = [];

        getShirts();

        function keypress(e, form) {
            if (e.which === 13)
                form.$submit();
        }

        function getShirts() {
            $http
                .post('api/get', {
                    action: 'display'
                })
                .then(successCallback, errorCallback);

            function successCallback(response) {
                vm.shirts = response.data;

                // Setting order number
                for (var i = 0; i < vm.shirts.length; i++)
                    vm.shirts[i].order = i + 1;
            }

            function errorCallback() {
                swal({
                    title: "NIEPOWODZENIE!",
                    text: "Pobieranie koszulek zakończyło się niepowodzeniem",
                    type: "error",
                    allowOutsideClick: true
                });
            }
        }

        function submitAction(index, data, id) {
            // If id is undefined add new shirt
            if (typeof id === 'undefined')
                return newShirt(data, index);

            updateShirt(data, id);
        }

        function addShirt() {
            vm.inserted = {
                order: vm.shirts.length + 1,
                team: '',
                cmd: '/',
                isNew: true
            };
            vm.shirts.push(vm.inserted);
        }

        function newShirt(data, index) {
            $http
                .post('api/add', data)
                .then(successCallback, errorCallback);

            function successCallback(response) {
                // API returns id of new object if added
                if (!isNaN(response.data[0].id)) {
                    vm.shirts[index].isNew = false; //if it was not set to false, would be deleted when "cancelForm" called
                    vm.shirts[index].id = response.data[0].id; //set a new id of object
                    vm.shirts[index].order = vm.shirts.length; // set order number

                    swal({
                        title: "Dobre wieści!",
                        text: "Nowa koszulka została zapisana",
                        type: "success",
                        allowOutsideClick: true
                    });
                } else {
                    vm.shirts.splice(index, 1); //remove shirt from list if not added to database
                    actionError();
                }
            }

            function errorCallback() {
                vm.shirts.splice(index, 1);
                actionError();
            }
        }

        function updateShirt(data, id) {
            angular.extend(data, {
                id: id
            });

            $http
                .post('api/update', data)
                .then(successCallback, errorCallback);

            function successCallback(response) {
                if (response.data == 1) {
                    swal({
                        title: "Dobra robota!",
                        text: "Koszulka została zedytowana!",
                        type: "success",
                        allowOutsideClick: true
                    });
                } else
                    actionError();
            }

            function errorCallback() {
                actionError();
            }
        }

        function removeShirt(index, id, team, cmd) {
            swal({
                    title: "Jesteś pewien?",
                    text: "Czy napewno chcesz skasować tę koszulkę?\n\nTeam: " + team + "\nCMD: " + cmd,
                    type: "warning",
                    showCancelButton: true,
                    confirmButtonColor: "#DD6B55",
                    confirmButtonText: "Tak, skasuj",
                    cancelButtonText: "Anuluj!",
                    closeOnConfirm: false,
                    closeOnCancel: false,
                    allowOutsideClick: true
                },
                function(isConfirm) {
                    if (isConfirm) {
                        $http
                            .post('api/delete', {
                                'id': id
                            })
                            .then(successCallback, errorCallback);
                    } else
                        swal({
                            title: "Anulowano",
                            text: "Koszulka pozostała niezmieniona :)",
                            type: "info",
                            allowOutsideClick: true
                        });

                    function successCallback(response) {
                        if (response.data == 1) {
                            // Remove in that way becouse of errors when array is custom sorted
                            vm.shirts.forEach(function(shirt, ind) {
                                if (shirt.id === id)
                                    vm.shirts.splice(ind, 1);
                            });

                            // Reordering shirts
                            for (var i = 0; i < vm.shirts.length; i++)
                                vm.shirts[i].order = i + 1;

                            swal({
                                title: "Skasowane!",
                                text: "Wybrana koszulka została usunięta.",
                                type: "success",
                                allowOutsideClick: true
                            });
                        } else
                            actionError();
                    }

                    function errorCallback() {
                        actionError();
                    }
                }
            );
        }

        function cancelForm(index) {

            // Simply cancelling form does not remove added record,
            // have to do this manually
            if (vm.shirts[index].isNew)
                vm.shirts.splice(index, 1);

            // Reordering shirts
            for (var i = 0; i < vm.shirts.length; i++)
                vm.shirts[i].order = i + 1;
        }

        function checkTeamField(data) {
            if (typeof data === 'undefined' || data.trim().length < 4)
                return 'Opis jest zakrótki';
        }

        function checkCmdField(data) {
            var pattern = new RegExp("^(/colors)\\s(blue|red)\\s");
            if (typeof data === 'undefined' || !pattern.test(data.trim()))
                return 'CMD jest niepoprawna';
        }

        function actionError() {
            swal({
                title: 'Ooops...',
                text: 'Nic z tego, gdzieś wdarł się błąd',
                type: 'error',
                allowOutsideClick: true
            });
        }
    }
})();

This is my index.html file:

<!DOCTYPE html>
<html ng-app="app">

<head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1">
    <title>Haxball Shirts</title>

    <!-- Reset Css -->
    <link href="/vendor/css/reset.css" rel="stylesheet">

    <!-- Bootstrap -->
    <link href="vendor/css/bootstrap.min.css" rel="stylesheet">
    <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/font-awesome/4.5.0/css/font-awesome.min.css">


    <script src="vendor/js/jquery.min.js"></script>
    <script src="vendor/js/bootstrap.min.js"></script>

    <!-- Main css -->
    <link href="assets/css/main.css" rel="stylesheet">

    <!-- Angular -->
    <script src="vendor/js/angular.min.js"></script>

    <!-- SweetAlert -->
    <script src="vendor/js/sweetalert.min.js"></script>
    <link href="vendor/css/sweetalert.css" rel="stylesheet">

    <!-- XEditable -->
    <script src="vendor/js/xeditable.min.js"></script>
    <link href="vendor/css/xeditable.css" rel="stylesheet">

    <!-- Module -->
    <script src="app.module.js"></script>

    <!-- Controllers -->
    <script src="controllers/manage.controller.js"></script>
</head>

<body>
    <div ng-controller="ManageController as manage">
        <div class="container">
            <div class="table-responsive">
                <table class="table table-hover">
                    <thead>
                        <th>
                            <a href="" ng-click="orderByField='nr'; reverseSort = !reverseSort">
                                Nr
                                <span ng-show="orderByField == 'nr'">
                                    <span ng-show="!reverseSort">
                                        <i class="fa fa-sort-numeric-asc"></i>
                                    </span>
                                    <span ng-show="reverseSort">
                                        <i class="fa fa-sort-numeric-desc"></i>
                                    </span>
                                </span>
                            </a>
                        </th>
                        <th>
                            <a href="" ng-click="orderByField='team'; reverseSort = !reverseSort">
                                Team
                                <span ng-show="orderByField == 'team'">
                                    <span ng-show="!reverseSort">
                                        <i class="fa fa-sort-alpha-asc"></i>
                                    </span>
                                    <span ng-show="reverseSort">
                                        <i class="fa fa-sort-alpha-desc"></i>
                                    </span>
                                </span>
                            </a>
                        </th>
                        <th>
                            <a href="" ng-click="orderByField='cmd'; reverseSort = !reverseSort">
                                CMD
                                <span ng-show="orderByField == 'cmd'">
                                    <span ng-show="!reverseSort">
                                        <i class="fa fa-sort-alpha-asc"></i>
                                    </span>
                                    <span ng-show="reverseSort">
                                        <i class="fa fa-sort-alpha-desc"></i>
                                    </span>
                                </span>
                            </a>
                        </th>
                        <th>Action</th>
                    </thead>
                    <tbody>
                        <tr ng-dblclick="rowform.$show()" ng-repeat="shirt in manage.shirts|orderBy:orderByField:reverseSort">

                            <td>
                                <span e-name="order" e-form="rowform">
                                    {{ shirt.order }}
                                </span>
                            </td>

                            <td>
                                <span editable-text="shirt.team" e-ng-keypress="manage.keypress($event, rowform)" e-name="team" e-form="rowform" onbeforesave="manage.checkTeamField($data)" e-required>
                                    {{ shirt.team }}
                                </span>
                            </td>

                            <td>
                                <span editable-text="shirt.cmd" e-ng-keypress="manage.keypress($event, rowform)" e-name="cmd" e-form="rowform" onbeforesave="manage.checkCmdField($data)" e-required>
                                    {{ shirt.cmd }}
                                </span>
                            </td>

                            <td style="white-space: nowrap">
                                <form editable-form name="rowform" ng-show="rowform.$visible" onbeforesave="manage.submitAction($index, $data, shirt.id)" class="form-buttons form-inline" shown="manage.inserted == shirt">

                                    <button type="submit" ng-disabled="rowform.$waiting" class="btn btn-primary">
                                        Save
                                    </button>
                                    <button type="button" ng-disabled="rowform.$waiting" ng-click="rowform.$cancel(); manage.cancelForm($index)" class="btn btn-warning">
                                        Cancel
                                    </button>

                                </form>

                                <div class="buttons" ng-show="!rowform.$visible">
                                    <button class="btn btn-primary" ng-click="rowform.$show()">Edit</button>
                                    <button class="btn btn-danger" ng-click="manage.removeShirt($index, shirt.id, shirt.team, shirt.cmd)">Delete</button>
                                </div>

                            </td>
                        </tr>
                    </tbody>
                </table>
                <button class="btn btn-default" ng-click="manage.addShirt()">Dodaj koszulkę</button>
            </div>
        </div>
    </div>
</body>

</html>

I can't give you a live demo because it requires a PHP back-end.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

First, I'd suggest you to use a package manager for all those front-end libraries. Bower is a great start, especially for front-end. npm is also good, but you'll have to read up first if it works for you.

Next, don't use a reset. You already use Bootstrap, and it already ships with normalize (version 4 comes with reboot). Normalize is less destructive than a reset, in that it only makes the defaults the same across browsers, and not totally stripping off the default styling. Reboot is a more opinionated stylesheet on top of normalize, used by Bootstrap 4.

Now your template is hardcoded into the HTML. Nothing wrong with it. But eventually, you'll move into multiple views. I'd suggest you start using Angular's routing. With routes, you match a path and provide it a template and controller at the same time. You can start by just matching root (/) then moving over everything.

Your code looks good, but I suggest you break them apart. A good guideline I follow is this:

  • Factories/services/providers - REST API calls, business logic. Also holds application state, especially when you start to share them across controllers.

  • Controller - UI-level logic like UI-level validations, computed properties etc. Must NOT call external APIs directly, pull in a service for that. Must NOT touch the DOM, should only expose data and functions. This keeps the controller independent of the template that uses it.

  • Template - UI, presentational logic (conditionally putting classes, styles etc.). Consumes scope data and calls scope functions. Templates are normally dependent on the controller they're bound to.

  • Directives - Common widgets (like rows of the table, button groups), the only part with DOM-touching code. If you need a jQuery plugin or something, wrap it in a directive. Ideally, directives only operate on whatever they're handed. Keeps them portable.

I suggest you start with the above. That should trim off your code. Additionally, while being verbose is fine, it's often (at least for me) very tedious to read and a lot of running around. For instance, the controller body can be inlined

var app = angular.module(...);

app.controller('controllerName', function(){
  // controller body
});

...or API callbacks

$http.post(...).then(function(){
  // success
}, function(){
  // fail
});

Verbose is good but concise is better. Less visual clutter. Looking at your code closer, your indents are inconsistent. I suggest you use an IDE or a text editor with a formatter. Sublime Text with jsbeautifier plugin works well for me. If you use something better (like Visual Studio or Intellij IDEA) then by all means use the built-in reformatting facilities.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks a lot. You mentioned about concise code - I found some nice Angular Style Guide github.com/johnpapa/angular-styleguide, when is written that verbose is better, and more readable. So I decided to put all my calls outside the calling functions as the author advices. The tutorial seems to be really good - 14k Stars. I use Atom Editor (I moved from Sublime) and it also offers some jsbeautifier plugin that I always use. Does my code is badly formatted (if you mentioned about this)? I am just getting into Angular world, and for now, I don't know much about directives/services etc. \$\endgroup\$
    – mrJoe
    Commented Dec 30, 2015 at 20:49

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.