Как провести рефакторинг и ревью кода?

Question

[okuznetsov1]

Написал код, помогите оценить его качество/стиль, и дать советы по улучшению

Код на здесь https://github.com/okuznetsov1/projects-angular/tr...

Answer 1

[Владимир]

www.codelord.net/2016/11/23/spotting-outdated-angu...
https://github.com/rwwagner90/angular-styleguide-es6

Сходу
1) Вместо $http используй $resource
2) getPriority, getTag, getStatus должны возвращать promise
3) Контроллер должен быть тоньше
4) $scope в контроллерах не использовать, за єто убивать нужно, вместо єтого используй ControllerAs

Comments

[okuznetsov1]

спасибо. сделал

[okuznetsov1]

По поовду "должны вовращать promise":

Таким вот образом организовать?

serviceDataByOptions
    .getPriority()
    .then(function(data) {                                                
        //vm.choices = [{id:'0c4dc671-b202-44a2-bf15-d94773cc1ccd',name:'Низкий'}];
        vm.choices = data;
        vm.priorityNew = '';

            serviceDataByOptions
                .getTag()
                .then(function(data) { 
                    vm.choicesTag = data;
                    vm.selectModelNew = {
                        tag: []
                    };

                        serviceDataByOptions
                            .getStatus()
                            .then(function(data) { 
                                vm.choicesStatus = data;
                                vm.statusNew = '';
                            })
                            .catch(function(error) {
                                console.log(error);
                            });

                })
                .catch(function(error) {
                    console.log(error);
                });

    })
    .catch(function(error) {
        console.log(error);
    });

[ozknemoy]

зачем использовать $resource?
$scope в контроллерах не использовать. смотря какая версия. если больше 1.5 то согласен

[Владимир]

ozknemoy: А зачем использовать старые версии на пет проджекте ? А по поводу первого, почитайте ссылки.

[ozknemoy]

я знаю ответ. мне ваш интересен

[Владимир]

Мой ответ - код короче и понятней.

[okuznetsov1]

Владимир, я благодарен вам за помощь, вы уже многое мне посоветовали. Но хотелось бы получить ещё один ваш совет - правильно ли я организовал "возврат promise" (см. чуть выше мой пример)? Предполагаю, что существуют и другие варианты?

[Владимир]

okuznetsov1: Я постараюсь найти пример правильной (как мне кажется) работы с $resource, в связке template/controler/service это все что я могу обещать.
В вашем коде мне не нравится то, что в блоке then присваивается

vm.selectModelNew = {
                        tag: []
                    };

Инициализация должна віполняться во кнструкторе контроллера
Вот это тоже
vm.statusNew = '';

Кроме того в блоке catch вы гасите ошибки
Кроме того вы просто присваиваете значения свойствам контроллера в блоках then, это нельзя назвать "возвращаю promise", я постараюсь в песочнице написать небольшой кусок как бы делал я, правда без вложености.

Ну и еще вот могу порекомендовать чтиво
https://habrahabr.ru/company/mailru/blog/269465/

[okuznetsov1]

>Кроме того в блоке catch вы гасите ошибки
Владимир, вы имеете ввиду выдавать ошибку при помощи throw (например так: throw new Error('description error');)

>по поводу ссылки
очень познавательная статья, понял трюк промисов, но трудновато применить к своему случаю, начал думать над этим.

>также по поводу "...небольшой кусок как бы делал я, правда без вложености" и "...$resource"
было бы здорово, если бы вы показали мне эти куски кода

[okuznetsov1]

Владимир, вы это имели в виду (по поводу промиссов):

serviceDataByOptions
                    .getPriority()
                    .then(function(data) {
                        return vm.choices = data;   
                    })
                    .then(function() {
                        serviceDataByOptions
                            .getTag()
                            .then(function(data) { 
                                return vm.choicesTag = data;
                            })
                            .catch(function(error) {
                                console.log(error);
                            });
                    })
                    .then(function() {
                        serviceDataByOptions
                            .getStatus()
                            .then(function(data) { 
                                vm.choicesStatus = data;
                            })
                            .catch(function(error) {
                                console.log(error);
                            });
                    })
                    .catch(function(error) {
                        console.log(error);
                    });

[Владимир]

Блок

.catch(function(error) {
                                console.log(error);
                            });

не нужен
Тут
return vm.choicesTag = data;
return не имеет смысла
Блоки
serviceDataByOptions.getPriority()
и
serviceDataByOptions.getTag()
и
serviceDataByOptions.getStatus()
Выполняются строго последовательно или в любом порядке
Судя по тому что нет входящих параметров, они могут выполняться в любой последовательности

[okuznetsov1]

Владимир, на этот раз надеюсь, что сделал всё как нужно?

try {
               
                    serviceDataByOptions
                        .getPriority()
                        .then(function(data) {
                            //vm.choices = [{id:'0c4dc671-b202-44a2-bf15-d94773cc1ccd',name:'Низкий'}];
                            vm.choices = data;   
                        })
                        .then(function() {
                            serviceDataByOptions
                                .getTag()
                                .then(function(data) { 
                                    vm.choicesTag = data;
                                })
                        })
                        .then(function() {
                            serviceDataByOptions
                                .getStatus()
                                .then(function(data) { 
                                    vm.choicesStatus = data;
                                })
                        })
                        .catch(function(error) {
                            throw new Error('Произошла ошибка в serviceDataByOptions. Возникли проблемы с получением данных из БД! ' + error);
                        });

                } catch(error) {
                    vm.notify('error',error.message);
                    console.log(error);
                }

Answer 2

[sutkin main]

1) код разбить по модулям
2) использовать es6
3)
if (type === 'simple')
{
LxNotificationService.notify(text);
}
else if (type === 'sticky')
{
LxNotificationService.notify('text', undefined, true);
}
else if (type === 'icon')
{
LxNotificationService.notify(text, 'android');
}
else if (type === 'color')
{
LxNotificationService.notify(text, undefined, false, 'grey');
}
else if (type === 'info')
{
LxNotificationService.info(text);
}
else if (type === 'success')
{
LxNotificationService.success(text);
}
else if (type === 'warning')
{
LxNotificationService.warning(text);
}
else if (type === 'error')
{
LxNotificationService.error(text);
}
switch case))) думаю тут будет куда лучше
4) Пытайся для начало писать код по принципу:
1 фунция решаюет 1 задачу а не так:
function dataProcessingIsTagsForDrawTable(){

var s;
var arrayTags = [];

//Собираем массив содержащий tag.name для его отображения в datatable
angular.forEach(vm.dataTableTbody, function (element) {
s = '';
angular.forEach(element.tags_id, function (tag) {
s += tag.tag_name + ', ';
});
s = s.substring(0, s.length - 2);
arrayTags.push(s);
});

angular.forEach(vm.dataTableTbody, function (element, key) {
element.tags_name = arrayTags[key];
});

}
в конце хочется сказать не бросай писать код, насколько больше пишешь кода настолько и больше будешь понимать как делать рефакторинг и review кода

Comments

[okuznetsov1]

Спасибо.

>По поводу "2) использовать es6":
вы имеете ввиду использовать anguler2 вместо angularJS?

>По 1) код разбить по модулям :
можете не много по подробней по этому пункту. в частности - какие части вынести в модули?

А вообще как хорошо по стилю, терпимо?