Problem w Aplikacji TO-DO i code review

Question

Witam!

Napisałem aplikacje TO-DO w ramach nauki języka javascript. Pojawił się problem który polega na tym że jeśli dodam za dużo elementów w liście zagnieżdżonej (jest lista i pod każdym podpunktem można dodać mniejszą liste) to aplikacja zaczyna lagować i potwornie wolno działać. Zaczyna to być zauważalne mniej wiecej przy 12-13 elementach podlisty. Jeżeli dodaje dużo elementów do głównej listy to nie ma tego problemu. Konsola nic nie wyświetla. Myśle że może to być problem z za dużą ilością addEventListener i że powinienem to zrobić delegacją zdarzeń jednak nie spodziewałem się takiego problemu w tak małej aplikacji. Z góry dziękuje za wszystkie rady  oraz prosze o konstruktywną krytyke mojego całego kodu :)

Link do aplikacji
https://kubawysocki.github.io/TO-DO/

Oraz link do repozytorium na GitHubie
https://github.com/KubaWysocki/TO-DO

Answer 1

Profilowanie ładnie pokazało winowajcę:

https://screenshotscdn.firefoxusercontent.com/images/46e07e71-ec82-4289-8bf9-959547d3a2a4.png

Widać, że ponad 90% czasu spędzamy w addEventListener(). Dlaczego?

    for (i=0;i<minus.length;i++){
        if(minus[i])minus[i].addEventListener('click',erase);
        if(allTic[i])allTic[i].addEventListener('click',ticAction);
        if(edit[i])edit[i].addEventListener('click',redoMode);
        if(addSubList[i])addSubList[i].addEventListener('click',subList);
        if(arrow[i]) arrow[i].addEventListener('click',hideSubList);
        if(subItem[i]) subItem[i].addEventListener('click',function(){addSubItem(this.previousSibling)});
        if(inputSubList[i])inputSubList[i].addEventListener('keydown',function(e){
            if(e.key=="Enter") addSubItem(this);
        });
}

Ten kod wielokrotnie dodaje event listenery robiące to samo do tego samego elementu - przy każdym nowym todo event handler wywołuje się więcej razy, więcej razy dodaje ten sam event listener etc.

Comments

Wielkie dzięki!

Już biorę się za przebudowe :)

Jak byś mógł mi powiedzieć jeszcze w jaki sposób to sprawdziłeś to byłbym wdzięczny :)

edit: Już przebudowałem i dodaje listnery odrazu przy tworzeniu elementów a funkcje makeActions (czyli tego bubla powyżej xD ) wykorzystuje tylko na onload przy dodawaniu listnerów do elementów wczytanych z local.storage :)

---

Jak byś mógł mi powiedzieć jeszcze w jaki sposób to sprawdziłeś to byłbym wdzięczny :)

AFAIK zarówno FF jak i Chrome mają teraz wbudowane profilery. Trzeba nauczyć się je czytać, ale ogólnie wystarczy mieć go włączonego i wywołać wolno działający kod.

Już przebudowałem i dodaje listnery odrazu przy tworzeniu elementów

Super :)

Answer 2

Dobra pierwsza sprawa - strona wygląda okropnie, ale dokładniejszą analizę UI zostawię innym, ja przejdę do kodu JS, który jest dosyć wątpliwej jakości, ale do rzeczy.

• Zero modułowości, całość kodu w jednym pliku, co strasznie utrudnia analizę tego co się w środku dzieje.
• Funkcje posiadają zdecydowanie za dużo odpowiedzialności

• this.parentNode.parentNode.parentNode.parentNode.removeChild(this.parentNode.parentNode.parentNode); //oraz podobne kwiatki rozsiane po kodzie

• Formatowanie kodu pozostawia wiele do życzenia

• Bardzo dużo powtórzeń kodu

Jak uprzątniesz kod to przejdziemy do kwestii wydajności, bo obecnie to nie ma sensu.
 

 

 

Comments

Wielkie dzięki za odpowiedź!

Zaczynając od początku to co do wyglądu to może i smutne ale nie wiedziałem że jest aż tak źle jak na początkującego :P 

• jeśli chodzi o modułowość to nie mam na ten temat jeszcze pojęcia 
• nie rozumiem co znaczy za duża odpowiedzialność funkcji- prosze o wyjaśnienie 
• jeśli chodzi o ten fragment kodu to chodzi ci o to że powinno to wyglądać tak 

  let element =this.parentNode.parentNode.parentNode.parentNode;
  let delElement= this.parentNode.parentNode.parentNode;
  element.removeChild(delElement); 

  czy raczej całość jest zła i powinienem kompletnie zmienić podejście?

• jeśli chodzi o formatowanie to myśle że przychodzi z czasem i oczywiście będę dalej nad tym pracował 

• a jeżeli chodzi o powtórzenia to najczęściej powtarza się 

  document.createElement('div')

  a to z tego powodu że nie wiedziałem jak utworzyć raz element div i później dodając klase do różnych divów zapisywać je w innych zmiennych. Poza tym powtarzają się jeszcze uchwyty do tego samego elementu w różnych funkcjach a to z tego względu że nie chciałem dodawać zmiennych w zakresie globalnym bo słyszałem że to zła praktyka. 

• mam jeszcze pytanie czy wiesz może skąd pojawiają się te poziome kreski i wcięcia w tym dużym pojemniku takie jak na zdjęciu

Pozdrawiam :)

---

Jeszcze dorzuce od siebie:

 Jeżeli możemy to używamy const, a nie let.

Po drugie, póki tego kodu nie rozbijesz na kilka plików, to raczej nikt tego nie sprawdzi.

 co do zdjecia wyzej: masz ustawiony jakis border radius

---

za duża odpowiedzialność = za dużo rzeczy do wykonania

---

@k.wichura, no tak mam border-radius ale czemu wcięcie pojawia się w środku elementu? Zaokrąglone powinny być tylko końce. Poza tym pojawiają sie te poziome linie i czasami jakieś małe przerwy między elementem listy a jego podlistą. Te rzeczy kojarzą mi sie z artefaktami bo pojawiają się nie zawsze a tylko w jakiś określonych okolicznościach... 

 

---

Ciężko mi odowiedzieć, bo u mnie na chromie tego nie ma, a w ie11 nie działa w ogóle.