• Najnowsze pytania
  • Bez odpowiedzi
  • Zadaj pytanie
  • Kategorie
  • Tagi
  • Zdobyte punkty
  • Ekipa ninja
  • IRC
  • FAQ
  • Regulamin
  • Książki warte uwagi

Ocena kodu JS

Object Storage Arubacloud
0 głosów
228 wizyt
pytanie zadane 10 sierpnia 2020 w JavaScript przez Avocado Nowicjusz (120 p.)

Cześć wykonałem sobie projekt z tej stronki: https://jsbeginners.com/javascript-projects-for-beginners/

Bardzo proszę o ocenę kodu JS :)

Repozytorium: https://github.com/Danie3L/-grocery-list

Live: https://hungry-goodall-519493.netlify.app/ 

Pozdrawiam.

 

komentarz 10 sierpnia 2020 przez jankustosz1 Nałogowiec (35,940 p.)
Nigdy nie pisałem nic większego w czystym js, ale wydaje mi się, że dobrze  jakbyś to porozbijał na różne pliki i klasy.

W sensie jedna stronka może i tak przejdzie, ale jakbyś chciał coś więcej to będzie potem ciężko debuggować

3 odpowiedzi

+2 głosów
odpowiedź 11 sierpnia 2020 przez Comandeer Guru (602,560 p.)
  • #1 – nie dołączasz swojego pliku JS jako moduł, a więc tworzysz zmienne globalne. Możesz to dołączyć jako moduł albo otoczyć całość w IIFE/blok.
  • #1-#2 – raz używasz kolekcji document.forms, a raz – querySelectorAll. Ustandaryzowałbym to i używał zawsze tego drugiego.
  • #20 – a skąd pewność, że input będzie zawsze pierwszy? Lepiej skorzystać z querySelector.
  • #38 – tu tak samo – skąd wiadomo, który rodzic to ten odpowiedni? Lepiej skorzystać z closest.
  • Ogólnie to podzieliłbym to sobie na model i widok. W sensie: wszystkie rzeczy trzymałbym w tablicy i jedynie tę tablicę wyświetlał
komentarz 11 sierpnia 2020 przez ScriptyChris Mędrzec (190,190 p.)
edycja 11 sierpnia 2020 przez ScriptyChris

#1-#2 – raz używasz kolekcji document.forms, a raz – querySelectorAll. Ustandaryzowałbym to i używał zawsze tego drugiego.

Orientujesz się, czy są jakieś wady stosowania metod propertisów document.[ forms/images/scripts/stylesheets ] vs document.querySelectorAll? Te pierwsze zdaje się, że zawsze zwracają (żywą) kolekcję HTML vs statyczny snapshot z querySelectorAll - czy poza tym są jakieś przeciwwskazania?

1
komentarz 11 sierpnia 2020 przez Comandeer Guru (602,560 p.)

Specyfikacja DOM wprost zaznacza, że takie kolekcje to "zaszłość historyczna". Ich się praktycznie nie rusza, nie są w żaden sposób rozwijane.

Dodatkowo po prostu dla spójności używałbym jednego sposobu pobierania elementów.

komentarz 11 sierpnia 2020 przez Avocado Nowicjusz (120 p.)
Dzięki za cenne rady , teraz używam wszędzie querySelector , użyłem closest , użyłem IIFE , oraz rozbiłem na dwie funkcje - dodawanie pojedynczej listy oraz usuwanie jej. Mógłbyś teraz zerknąć ? I nie do końca rozumiem ostatni podpunkt twojej wypowiedzi , mógłbyś to jakoś rozwinąć ? Repo: https://github.com/Danie3L/-grocery-list
komentarz 12 sierpnia 2020 przez Comandeer Guru (602,560 p.)

teraz używam wszędzie querySelector

 https://github.com/Danie3L/-grocery-list/blob/master/js/app.js#L2-L5 – hmm…

I nie do końca rozumiem ostatni podpunkt twojej wypowiedzi , mógłbyś to jakoś rozwinąć ?

 Chodzi mi o rozdzielenie widoku od warstwy prezentacji. Zamiast mieć listę produktów wyłącznie w DOM, jako treść elementów, lepiej mieć po prostu tablicę w kodzie, która będzie je przechowywać. I jak coś do tej tablicy zostanie dodane lub z niej usunięte, to uaktualnić listę na stronie.

0 głosów
odpowiedź 10 sierpnia 2020 przez niezalogowany
Strona mi się Bardzo Podoba !!!

Kodu nie ocenię bo jestem początkujący....

Pozdrawiam Serdecznie;)
0 głosów
odpowiedź 11 sierpnia 2020 przez Else Stary wyjadacz (12,260 p.)

Oceniłem tylko plik app.js. Przeczytałem cały kod i wydaje mi się oczywisty, czyli taki jaki powinien być czysty kod. Jest jedno miejsce gdzie podczas czytania czułem się trochę zagubiony. Chodzi o linijki: 

 const groceryListBox = createGroceryListBox(userInputValue);
    const listItems = document.querySelector(".list-items");
    listItems.insertAdjacentHTML("beforeend", groceryListBox);
    const removeIcons = [...document.querySelectorAll(".fa-trash")];
    removeIcons.forEach((icon) => icon.addEventListener("click", removeBox));

Na moje oko mieszasz tu warstwy abstrakcji. Powinieneś odseparować takie nisko poziomowe sprawy jak 

document.querySelector(".list-items");

czy 

const removeIcons = [...document.querySelectorAll(".fa-trash")]; removeIcons.forEach((icon) => icon.addEventListener("click", removeBox));

od logiki biznesowej. Mówiąc w skrócie te elementy mógłbyś ukryć w jakichś funkcjach, żeby dobrze się czytało ten kod.

Polecam Ci zobaczyć: 

https://www.youtube.com/watch?v=7EmboKQH8lM&list=WL&index=14

Podobne pytania

+1 głos
2 odpowiedzi 214 wizyt
pytanie zadane 8 lutego 2020 w JavaScript przez Dev26 Nowicjusz (130 p.)
0 głosów
1 odpowiedź 388 wizyt
pytanie zadane 21 sierpnia 2017 w JavaScript przez Marchiew Dyskutant (7,690 p.)
+2 głosów
3 odpowiedzi 900 wizyt
pytanie zadane 9 lipca 2017 w JavaScript przez Ziken Początkujący (330 p.)

92,674 zapytań

141,578 odpowiedzi

320,046 komentarzy

62,039 pasjonatów

Motyw:

Akcja Pajacyk

Pajacyk od wielu lat dożywia dzieci. Pomóż klikając w zielony brzuszek na stronie. Dziękujemy! ♡

Oto polecana książka warta uwagi.
Pełną listę książek znajdziesz tutaj.

Akademia Sekuraka

Niedawno wystartował dodruk tej świetnej, rozchwytywanej książki (około 940 stron). Mamy dla Was kod: pasja (wpiszcie go w koszyku), dzięki któremu otrzymujemy 10% zniżki - dziękujemy zaprzyjaźnionej ekipie Sekuraka za taki bonus dla Pasjonatów! Książka to pierwszy tom z serii o ITsec, który łagodnie wprowadzi w świat bezpieczeństwa IT każdą osobę - warto, polecamy!

...