Problem w Aplikacji TO-DO i code review

pytanie zadane 25 października 2018 w JavaScript przez Jestem_Szaleńcem Użytkownik (530 p.)
edycja 25 października 2018 przez Jestem_Szaleńcem

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

2 odpowiedzi

odpowiedź 25 października 2018 przez adrian17 Mentor (354,880 p.)

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.

komentarz 25 października 2018 przez Jestem_Szaleńcem Użytkownik (530 p.)
edycja 25 października 2018 przez Jestem_Szaleńcem

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 :)

komentarz 25 października 2018 przez adrian17 Mentor (354,880 p.)

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 :)

odpowiedź 25 października 2018 przez ProgramistaStepek Nałogowiec (27,020 p.)

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.

komentarz 25 października 2018 przez Jestem_Szaleńcem Użytkownik (530 p.)
edycja 25 października 2018 przez Jestem_Szaleńcem

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 :)

komentarz 25 października 2018 przez k.wichura Pasjonat (19,870 p.)
edycja 25 października 2018 przez k.wichura

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

komentarz 25 października 2018 przez ProgramistaStepek Nałogowiec (27,020 p.)

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

komentarz 25 października 2018 przez Jestem_Szaleńcem Użytkownik (530 p.)

@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...