patternjavascriptMinor
Angular.JS table management
Viewed 0 times
managementangulartable
Problem
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.
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
This application manages the data from database, shows it in table, allows to add new rows, edit existing records and delete them.
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
Solution
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 (
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
...or API callbacks
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.
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.
Code Snippets
var app = angular.module(...);
app.controller('controllerName', function(){
// controller body
});$http.post(...).then(function(){
// success
}, function(){
// fail
});Context
StackExchange Code Review Q#115454, answer score: 3
Revisions (0)
No revisions yet.