Ревью кода. Что можно улучшить в этом коде?

Question

[Ivan2507]

Здравствуйте, я изучаю front-end разработку 2 .5 месяца, решил перейти на практику и написал todo list за часов 8
1) Напишите, пожалуйста, свое мнение о коде и как его можно улучшить (желательно по html, css тоже, но в приоритете JS)
2) Хочу начать изучение React, уже можно начинать или лучше ещё попрактиковаться на JS?

Ссылка на код: https://codepen.io/domarchuk77/pen/xxRoMBY

Answer 1

[Sun_Day]

Честно говоря, это плохой код. Смысла разбираться в нем не имеет, долго объяснять все моменты(все это просто придется переписать под корень). Но все новички в программировании пишут что-то подобное, это нормально.

Могут отметить несколько вещей:
1) Нейминг css классов - почитайте про БЭМ, у вас что-то невразумительное. Да и БЭМ тут не нужен, если прям строго взглянуть - у него свои задачи.
2) Используйте строгое равенство ===
3) Условия внутри методов просто кошмар. В целом в методах спагетти код по работе с dom. Это так не делается. Нужно декомпозировать логику и писать лаконичный и выразительный код.
4) Что-то можно было передать через constructor(), при создании экземпляра класса. Зачем это все пихать в сам constructor.

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

В общем, никуда это не выкладывайте всерьез). Ну и советую использовать тайпскрипт.

Реакт можно изучать конечно.

Answer 2

[Сергей Соколов]

В класс Task впихнули всё. А лучше бы как-то разделить: вот Task, их может быть ни одного или несколько. Вот App – это приложение, форма создания новой задачи; может, обработчик всех событий, которые всплывают из задач, в т.ч.; коллекция созданных задач; их сохранение в LocalStorage; отрисовка части задач в соответствие с фильтром.

Внутри класса у методов общий префикс названия taskЧто-то-там – лишний, имхо.

В коде жосско прописаны названия классов элементов, с которыми работать, где искать, и т.п. Может, лучше делать класс независимым от разметки и передавать в него уже созданные элементы. Если же элементы создаются внутри класса, сохранять референсы к ним.

p.s. React да, можно. Или Vue.

Answer 3

[AlbertEnshtein]

1) не ставишь точки с запятыми, вместо if-else использовать switch case, когда есть условия не выполнять какую-та функцию, надо из него поскорее выходить, например в функции filterCheckActive можно сделать так:

filterCheckActive(){
      if(this.filter.value != "active") return;        
            this.itemProgress.classList.remove('show');
            this.itemProgress.classList.remove('hide');
            if(this.itemProgress.classList.contains('task-complete')){
                this.itemProgress.classList.add('hide');
              return;
            }
                this.itemProgress.classList.remove('show');                  
    }

2) думаю можно начинать, удачи!)

Comments

[Сергей Соколов]

1) – там есть кнопка Format JavaScript – я сразу туда ))

Answer 4

[Алексей Джо]

Бем неправильный, нарушает концепцию - элемент не может содержать других элементов. Дальше смотреть не стал, уверен, что ошибок больше.