Code review projektu

[Willaiem]

Cześć, widziałem twój post na grupie fb dla stażystów i juniorów i postanowiłem sprawdzić jakość twojego kodu.

Rady dot. całej aplikacji:

• unikaj korzystania z export default, gdyż podczas importowania możesz dać dowolną nazwę, co w większości przypadków nie ma w ogóle sensu.
• unikaj korzystania z typu any, bo to podważa jakikolwiek sens korzystania z TypeScriptu - jak już musisz to korzystaj z typu unknown,
• nie wrzucaj do repo nieużywanego kodu - patrz: feature.js
• unikaj pisania komentarzy, gdyż one tylko utrudniają zrozumienie twojego kodu. Wyjątkiem są złożone RegExpy.
• jak tworzysz typy dla danego komponentu to lepszym pomysłem jest trzymanie zadeklarowanego typu wraz z komponentem/funkcją: Zamiast robić tak:

  
  import { ListButtonProps } from "../types/types";

const Button = ({ id, title, setCardType }: ListButtonProps) => { // ... }

to możesz zrobić tak:

type ListButtonProps = { // ... }

const Button = ({ id, title, setCardType }: ListButtonProps) => { // ... }

W ten sposób nie musisz przeskakiwać między plikami i ułatwia to modyfikację typów.
Jeśli musisz jakiś typ współdzielić między plikami to wtedy wrzucenie do ./types ma sens.

- przy `font-size` unikaj korzystania z pikseli ze względu na accessibility - lepiej jest używać jednostki `rem`.
Dołączam świetny artykuł na ten temat:
https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/
- w folderze components lepiej byłoby rozdzielić każdy komponent do osobnego folderu:
Dołączam dwa ciekawe artykuły na temat architektury folderów w React:
https://dev.to/profydev/screaming-architecture-evolution-of-a-react-folder-structure-4g25
https://www.joshwcomeau.com/react/file-structure/

App.tsx
`const [cardType, setCardType] = useState<number>(0)` - nie musisz tutaj korzystać z generyków w przypadku typów primitive, TypeScript sam zainferuje typ, który wprowadziłeś do funkcji useState i staraj się jak najczęściej z tego korzystać.

Flashcard.tsx
- funkcja imgHandler:
1. Z tego co widzę to ta funkcja na podstawie liczby zwraca ścieżkę do odpowiedniej ikony. Zatem ta funkcja mogłaby się nazywać "getLangIconSrc" bo sama nazwa imgHandler zupełnie nic mi nie mówi.
2. cardType jako cyfra? Dla mnie to jest dosyć dziwne i bugogenne rozwiązanie - wystarczy że w kodzie zmienię cyfrę i już nie będzie to działać. Lepiej jest już korzystać z unii przygotowanych nazw, na przykład:

type CardTypes = 'js' | 'css' | 'html' | 'react'

3. w default niczego nie zwracasz a wypisujesz wyłącznie do konsoli a wątpię aby użytkownik tam zajrzał. Lepszym pomysłem jest zwrócenie jakiegoś placeholdera.
- funkcja classHandler - tak samo jak w imgHandler,

MobileList.tsx
- funkcja toggleDrawer:
1. To co tutaj robisz jest dla mnie zupełnie niezrozumiałe - nie jestem w stanie pojąć, dlaczego jest to tak przekombinowane.
Wystarczyło stworzyć stan z prostą flagą isDrawerOpen i praktycznie samo by się to rozwiązało.
- również nie rozumiem, po co jest ta klamra oraz React.Fragment - to jest zupełnie niepotrzebne. Mam wrażenie, że nie do końca rozumiesz, po co jest właściwość `key` w Reakcie.
Ten artykuł może to ci rozjaśnić: https://kentcdodds.com/blog/understanding-reacts-key-prop
Jak i ten tutorial: https://beta.reactjs.org/learn/rendering-lists
- funkcja (a w zasadzie komponent) list:
1. Nie rozumiem, dlaczego zrobiłeś z tego komponent? Przede wszystkim unikaj takiego zagnieźdzania komponentów, bo to wpływa na wydajność Reacta + jest to po prostu nieczytelne i niezbyt praktyczne rozwiązanie

Ogólnie uważam, że ten komponent jest zupełnie niepotrzebny i mogło to być wszystko zaimplementowane w QuestionsList zamiast sztucznie to rozbijać na dwa komponenty.

QuestionsList.tsx
- od komentarza  //CARDS HANDLE SECTION do //END CARDS HANDLE SECTION mogłeś zrobić custom hooksa, aby rozdzielić wyświetlanie widoku (HTML) od logiki.
Ten film może pomóc w zrozumieniu tej koncepcji: https://www.youtube.com/watch?v=6ThXsUwLWvc  
bądź tekstowo: https://reactjs.org/docs/hooks-custom.html
- handleCardType ma takie same problemy co w imgHandler.
- isActive sugeruje, że to jest boolean, a okazuje się być liczbą. Jak już chcesz tak pozostawić to proponuje zmienić nazwę na np. activeCardId
- nie rozumiem w ogóle tej linijki kodu:
`flashcards.map((item: any) => (`
Po co typujesz item jako `any` skoro flashcards już jest otypowane? Nie rozumiem takiej decyzji.

data.tsx
- nazwa jest zbyt ogólnikowa
- nie ma sensu, aby znajdowała się w utils - lepszym miejscem byłby np. folder questions
- body jest bardzo dziwnie zrobione.
1. Nie używaj `<li>` poza elementami `<ul>` i `<ol>`
2. `<br>` mogłyby mieć jakiś sens, ale tutaj jest to trochę przekombinowane
3. Moim zdaniem, taka struktura sementycznie wyglądałaby dużo lepiej:

<>

Obiekty Promise zostały wprowadzone od ECMAScript 6 jako natywne wsparcie dla operacji asynchronicznych. Dzięki nim można niejako odłożyć wykonanie pewnej logiki na bok i zająć się głównym przepływem aplikacji. Przetwarzanie metody asynchronicznej może zakończyć się powodzeniem – wtedy wywołujemy metodę resolve() lub porażką – wtedy metodę reject();

      <p>Obiekt ten zawsze występuje w jednym z trzech stanów:</p>
      <ul>
        <li>
          Pending - wywołanie zostało zainicjowane ale jeszcze nie ukończone
        </li>
        <li>Fullfilled - .then() - wywołanie zakończone sukcesem</li>
        <li>Rejected - .catch() - wywołanie zakończone porażką</li>
      </ul>
      <p>Na obiekcie Promise zwróconym przez metodę możemy wywołać dwie metody:
      then odpowiedzialną za przetwarzanie udanego wywołania; catch
      odpowiedzialną za obsługę błędów.</p>
    </>

4. Ewentualnie mogłoby być to zrobione jako Markdown i załadować to za pomocą React Markdown.

README.md
- masz wypisany taki stack:
Stack:
1. React
2. Typescript
3. SCSS
4. RWD
5. Hooks
6. Material UI
7. Optimization Care

In future:
8. Redux Toolkit
9. Arrow navigation

tylko, że podpunkty 4,5,7 i 9 nie są technologiami.
Jak już chcesz wypisać to w taki sposób:
- React
- Typescript
- SCSS
- Material UI

In future
- Redux Toolkit (chociaż tutaj nie widzę jakiegokolwiek sensu korzystania z niego)

Podsumowując - trochę przekombinowałeś przy pisaniu tej apki, dodatkowo nie do końca korzystasz z mocy TS'a bo wiele razy widziałem typ `any` albo niepotrzebne generyki.
Mam wrażenie, że to jest twoje pierwsze starcie z TypeScriptem + React i stąd są takie problemy.

Mogę podrzucić ci parę materiałów jak napiszesz na Discordzie Will4_U#6954 bądź na facebooku (skomentowałem twój post pisząc o twoim cv).

Trudno ocenić samą apkę, bo jest bardzo prosta.
Pozdrawiwam.

[karton92]

Cześć Willaiem! Wielkie dzięki za to code review! Bardzo dużo cennych wskazówek. Nie wszystko udało mi się jeszcze poprawić, ale myślę, że jest już znacznie lepiej niż było. Faktycznie wkradło się sporo głupich błędów - zdecydowanie brak doświadczenia, pewnie troszkę gapiostwo.

Z Reactem to nie mój pierwszy projekt, ale co do Typescripta faktycznie masz racje. Momentami czuje się jeszcze zagubiony (na szczęście z każdą linijką kodu co raz mniej!) i pewnie jak wszyscy na początku drogi z nim - uważam, że na chwilę obecną troszkę bardziej przeszkadza niż pomaga. Oby jak najszybciej ten etap minął i obym mógł w pełni cieszyć się jego dobrodziejstwem :)

Ten projekt to moje pierwsze starcie z TS'em. Ciężko było/jest, ale mam wrażenie, że idzie to sukcesywnie do przodu. Nieocenione są tu podpowiedzi ESLinta i Quick Fix ;) Bardzo się cieszę, że jakiś czas temu to odkryłem, a jednocześnie bardzo żałuje, że nie wcześniej.

Aplikacja z założenia miała być prosta i bez fajerwerków - pierwsze skrzypce ma tu grać po prostu Typescript i jego nauka na własnym projekcie, a więc poprzeczka wyżej niż tutoriale z internetu :)