LINUX.ORG.RU

Поругайте код

 ,


1

3

Написал небольшое приложение(первое на Ангуляре)

И если честно, немного не уверен в коде, в частности в нескольких местах делал вещи с помощью jQuery хотя уверен должны быть локальные(ангулярские) методы для этого.

Исходники тут: https://github.com/Neoromantique/simple_angularjs_app
Live:
http://neoromantique.co.uk/shopping/

★★★

angularjs, javascript

Исходники тут

исходники не нужно было выкладывать, достаточно тегов.

emulek
()

не к ангуляру, но вообще. ты линтером код прогоняешь?

вот тут переменная без объявления. https://github.com/Neoromantique/simple_angularjs_app/blob/master/js/controll...

я тут на работу пытался устроиться. Так на меня товарисч смотрел как на немного непорядочного из за того что я линтер не запустил.. я попробовал - впринципе юзабельно, такие вот проблемки отлавливает.

Обн. или так модно нынче, у тебя что то там и везде без вар..

AndreyKl ★★★★★
()
Последнее исправление: AndreyKl (всего исправлений: 1)

В целом по яваскрипту у меня вот что:

L33

item.Name != null & item.Name != ''

По-моему идеоматически будет ожидать что у тебя либо число, либо всё остальное, и проверка такого рода сокращается до просто

if(item.Name) {
или если ты конвертишь в булевский, то
return !!item.Name
к примеру.

Т.е. в итоге 33 строка должна выглядеть проще, вместо

if (item.id == null & item.Name != null & item.Name != '') {
if (!item.id && item.Name) {

если же ты правда хочешь именно проверки на нулл, надо юзать === вместо ==. Потому что то что у тебя сейчас написано будет выполняться как

!item.id && item.Name
а не как проверка на нулл. кстати не замечал чтобы кто то кроме тебя юзал побитовое сложение вместо булевского.

А по ангуляру в основном по организации кода.

Я бы объявил отдельный файл services.js и отдельный файл controllers.js.

Каждая страница - отдельный контроллер (у тебя сейчас один - должно быть два, соотвественно). Каждый сервис - отдельный сервис. Т.е. должно быть минимум два сервива - categories и items.

Кроме прочего я засунул бы в файл сервайсес сервис мессаджей и инйектил бы его и дёргал бы alertShow этого сервиса. как то более порядочно получается. это я про L14 и в целом про то что сервисы в файле контроллеров. может у тебя тестовое приложение, но ты критики хотел, а в ангуляре так не принято делать (потому что тестирование затрудняется значительно), так что наверное в кассу.

Кроме того фильтрация очень просто делается фильтрами ангуляра, то я про L119

AndreyKl ★★★★★
()
Последнее исправление: AndreyKl (всего исправлений: 1)

Не смотрел, но осуждаю.

JN
()
Ответ на: комментарий от AndreyKl

Действительно, стоило вынести в 2 файла сервис и контроллер(я банально забыл, если честно).

Не уверен стоило бы или нет 2 контроллера делать, правда, т.к они очень взаимосвязаны.

А как «if (!item.id && item.Name) {» отреагирует на item.Name = ' '?

shuck ★★★
() автор топика
Ответ на: комментарий от shuck

Не уверен стоило бы или нет 2 контроллера делать, правда, т.к они очень взаимосвязаны.

я думаю, как минимум два контроллера и два сервиса. на мой взгляд связь там сиюминутная. дублирования кода не будет практически. проидись по ангуляровскому оф. туториалу, там это красной нитью проходит (из за тестирования).

Обн.

отреагирует на item.Name = ' '?


а как твой код отреагирует на это? так и мой.

соврал. не знал. но обычно всё таки принято тримать.

Обн2.

Тем не менее реагирует одинаково вроде, нет?

AndreyKl ★★★★★
()
Последнее исправление: AndreyKl (всего исправлений: 4)

Я бы везде вместо == использовал ===. А так вроде ничё.

anonymous
()
Ответ на: комментарий от shuck

дык, вроде как это не обязательно, могу быть не прав.

используется глобальная i тогда, не порядок.

AndreyKl ★★★★★
()
Ответ на: комментарий от shuck

ах, ну и да, общее мнение то :)

код слегка неидеоматичный, но вполне нормальный на мой вкус.

AndreyKl ★★★★★
()

Зачем ты объявляешь переменную i в качестве глобальной? Например тут:

this.deleteCategory = function(id) {
            for (i in cList) {
                ...
            }
}

Ja-Ja-Hey-Ho ★★★★
()
Последнее исправление: Ja-Ja-Hey-Ho (всего исправлений: 1)
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.