Siemanko. Uczę się Reakta po godzinach i zrobiłem kalkulator, bez zaglądania do jakichkolwiek tutoriali o robieniu kalkulatorów. To mój pierwszy kalkulator, w czystym jsie jakoś nigdy nie zrobiłem żadnego. Tutaj repo. Chętnie usłyszę zarówno obiektywną jak i subiektywną krytykę, więc piszcie śmiało. :)
0
1
Ogólnie to nie czepiając się:
- dziwnego sposobu nazywania katalogów, oraz komponentów
BUTTONS,RESULT.jsx, - nietypowego nazewnictwa zmiennych i funkcji jak na JavaScript
allow_modyfing_result_and_using_it_as_next_first_number,mark_current_display_as_result_to_allow_modyfing_result_and_using_it_as_next_first_number - formatowania css
* {
/* ... */
} html {
/* ... */
} body {
/* ... */
}
Bo widzę, że trzymasz sie tej konwencji sumiennie we wszystkich plikach i z przymrużeniem oka można powiedzieć, że jest okej to zmieniłbym sposób korzystania z reducerów. Wydziełbym je do osobnych plików i lekko rozbudował, żeby uprościć korzystanie z tego
const PRESS = 'press';
const ANIMATE = 'animate';
const buttonReducer = (state, action) => {
switch (action.type) {
case PRESS:
return {
...state,
value: action.value,
isPressed: action.value.length > 0,
};
case ANIMATE:
return {
...state,
isAnimated: action.value,
};
default:
return state;
}
};
const useButtonReducer = () => {
const [buttonState, dispatch] = useReducer(buttonReducer, {
value: '',
isPressed: false,
isAnimated: false,
});
const pressButton = (value) => {
dispatch({ action: PRESS, value });
}
const animateButton = (value) => {
dispatch({ action: ANIMATE, value });
}
return {
buttonState,
pressButton,
animateButton,
}
}
Wtedy korzystałbyś z tego w taki sposób
const { buttonState, pressButton, animateButton } = useButtonReducer();
// Ustawienie wartości "value" i "isPressed"
pressButton('...');
// Przekazanie pustego stringa resetuje "value" i ustawia "isPressed" na false
pressButton('');
// Włączenie animacji
animateButton(true);
// Wyłączenie animacji
animateButton(false)
// Zmiana stanu animacji na podstawie aktualnej wartości
animateButton(!buttonState.isAnimated);
zamiast tego co masz obecnie.
1
Cześć,
- Poczytaj o
floating point math
0.1 + 0.2 = 0.30000000000000004
-
Dziwna struktura projektów oraz nazewnictwo funkcji/propsów.
-
Klucz jako index. Może w tym przypadku nie ma to zbytniego znaczenia, ale podczas jakichś CRUDów możesz sobie narobić bigosu.
https://pl.reactjs.org/docs/lists-and-keys.html
-
Nazywaj funkcje/zmienne semantycznie. Co robi
press_button? To boolean? Flaga? Funkcja? Zmienna? Nie wiem, choć się domyślam. Jaka byłaby tu dobra nazwa? Myślę, żeonButtonClickalbo po prostuonClick
{is_being_animated ? 'calculator-button row' + row + ' active' : 'calculator-button row' + row}
można skrócić do
{`calculator-button row-${row} ${is_beign_animated ? 'active' : ''}`}
-
W
app.jsuseEffect na ponad 130 linijek. Nawet nie chcę na niego patrzeć, sorki :-P -
Tego nie rozumiem
{button_names.map((button, i) => {
return (
<BUTTON
name={button_names[i].name}
row={button_names[i].row}
is_being_animated={
button_being_animated &&
button_being_animated == button_names[i].name
? true
: false
}
press_button={press_button}
key={i}
></BUTTON>
);
})}
Dlaczego iterując po button_names odwołujesz się w przypadku row oraz name do button_names[i] zamiast do obecnie iterowanego obiektu, czyli button? Mógłbyś elegancko pyknąć name={button.name} albo już na siłę się przyczepiając to mógłbyś zrobić destrukt:
{button_names.map((button, i) => {
const {name, row} = button
return (
<BUTTON
name={name}
row={row}
[...]
I dalsza kwestia propsa is_beign_animated - mógłbyś to uprościć:
is_being_animated={button_being_animated === button_names[i].name}
Bo nawet w przypadku gdy button_beign_animated będzie undefined to nic się nie wykrzaczy.
- Wgraj sobie jakiś formatter, eslinta/prettiera czy coś co pomoże Ci ogarnąć kod, bo jest straszny miszmasz.
No i cała reszta, jak kolega wyżej napomknął. Pozdro
0
Dziękuję Wam za poświęcony czas i cenne rady. Warto było poprosić o review.
mister_tornister napisał(a):
- W
app.jsuseEffect na ponad 130 linijek. Nawet nie chcę na niego patrzeć, sorki :-P
Może jednak podejmiesz wyzwanie i zerkniesz? Najwyżej szybko zamkniesz kartę jak będzie za ostro. :)
1
Jeśli chodzi o ten kod z useEffect to często wewnątrz niego tworzysz jakieś pomocnicze funkcje, których używasz tylko raz
const dispatches = (value) => {
calculation_dispatch({type: COUNT, value});
calculation_dispatch({type: DISPLAY, value});
calculation_dispatch({type: OPERATION, value: null});
}
switch(operation) {
// tutaj ustawiasz wartość zmiennej value_to_display
}
dispatches(value_to_display); // pierwsze i ostatnie wywołanie funkcji dispatches
const change_display_value = () => {
const pressed_button = press_button_state.button_being_pressed === '0' ? 0 : press_button_state.button_being_pressed;
calculation_dispatch({
type: DISPLAY,
value: display_value(pressed_button)
});
}
// ... kilkanaście linijek później ...
change_display_value(); // tutaj tak samo pierwsze i ostatnie wywołanie funkcji change_display_value
więc na pewno utrudnia to debugowanie jeśli nie zna się kodu, bo raptownie trzeba przewijać cały kod hooka i sprawdzać skąd się to bierze.
Najlepiej byłoby jakby całe useEffect miało kilkanaście linijek żeby była jasność co od czego zależy
useEffect(() => {
if (...) {
actionA(value);
} else if (...) {
actionB(value);
actionC();
}
actionD();
}, [value])
i zależności związane z reducerami były przeniesione bezpośrednio do osobnego pliku, podobnie jak przekazałem w pierwszym poście.
1
Poza tym, co napisał @Xarviel (dziwaczny styl jak na CSS - BUTTONS, underscore'y zamiast camelCase itp.), to przemyślałbym pomysł implementacji tego kalkulatora w Redux. W tej chwili jest to straszne nieczytelne, wręcz przeinżynierowane.
No i po co ci ten Redux, jeśli i tak w zasadzie całą logikę masz w komponencie w useEffect i w do_operation. Redux ci tylko służy do trzymania raptem kilku zmiennych, a przecież to równie dobrze możesz robić używając React.useState. Ba, nawet mógłbyś mieć reducer w React używając useReducer.
Ogólnie więc moim zdaniem użycie Reduxa tutaj nie ma żadnego większego sensu.
Poza tym:
mark_current_display_as_result_to_allow_modyfing_result_and_using_it_as_next_first_number
Szacun za taką nazwę funkcji. Serio.
W sensie, że to nie jest dobra nazwa funkcji, raczej krzyczy TODO! Zrefaktoruj mnie.
Ale z dwojga złego lepiej dać nazwę super verbose (która będzie zwracała uwagę, że coś jest nie tak) niż nazwać zmienną skrótowo, a potem nie wiedzieć, co robiła.
Tym niemniej taka długa nazwa funkcji to wygląda jak syndrom tego, że gubisz się we własnym kodzie. Z tego, co widzę, to za pomocą tej nazwy enkapsulujesz po prostu to wywołanie:
calculation_dispatch({type: IS_RESULT_DISPLAYED, value: true});
I w tym jest problem. Ale myślę, że jest to objaw tego, o czym pisałem - że Redux jest na siłę wrzucony, a jak jest wrzucony, to trzeba robić dispatche, więc jak trzeba robić dispatche, które nie wiadomo czemu służą, to kod staje się niejasny, stąd potrzeba skomentowania kodu (tutaj przez samą nazwę funkcji).
Możliwe, że też nie przemyślałeś do końca logiki kalkulatora - tutaj mógłbyś napisać to najpierw jak najprościej, nawet napisać konsolowy program, który działa jak kalkulator i dopiero potem przeportować na Reacta - w idealnym przypadku napisać tak, żeby jeden program działał i w konsoli i w React. W zasadzie wtedy można by nawet rozważyć użycie tego Reduxa - bo jakbyś faktycznie napisał to w Redux i miał logikę w reducerach, to taka apka działałaby w konsoli, mógłbyś też napisać testy do tego (czyli dispatchował różne akcje symulujące działania użytkownika i sprawdzał czy stan store'a jest zgodny z oczekiwanym). A potem dopiero mógłbyś to podłączyć pod Reacta jako gotową apkę.