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

angularjs, javascript

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Обн.

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

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

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

Обн2.

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

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

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

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

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

Проверил, да, так-же.

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

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

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

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