@Kerubyte przejrzałem. Moje uwagi są wyrywkowe, błędów jest szczerze mówiąc za dużo, żeby wszystkie wyłapać i wytknąć (to u początkującego normalne).
##Git
Pojawiło się kilka nowych commitów, dalej będe wdzięczny za zerknięcie :)
Z ich nazewnictwem jest, szczerze mówiąc, jeszcze gorzej niż było.
Ostatni nazywa się "move timeNow from repo". I to jest treść w pełnym brzmieniu, żadnej "listy zakupów" już pod tym nie ma.
Zmiany, które się na niego składają, nie mają z tym kompletnie nic wspólnego.
Np. doszła implementacja OrderAdapter
... jakieś zmiany w UI...
Co to ma do owego timeNow
? Które, nawiasem mówiąc, nie zostało przesunięte z żadnego repo, bo było w obiekcie Utils
.
Zdarzało mi się obejrzeć "Kuchenne rewolucje" z Gessler. Zawsze dziwiłem się właścicielom, że np. nie wyszorują porządnie kuchni (nawet jeśli na co dzień tego nie robią). Nie mogą przecież nie wiedzieć, że baba właśnie to im wytknie, skoro robi to prawie zawsze, i męczyła o to bułę w większości poprzednich odcinków.
##Architektura
Jest 2021 i radziłbym przejść na data binding.
I tak używasz już przecież ViewModeli.
Jeżeli w takim kodzie widać wywołania findViewById
- czyli aplikacja aktualizuje interfejs "na piechotę" - to tak, jakby kupić nowy rower, ale na niego nie wsiadać, tylko prowadzić : )
Kod nie jest dobrze podzielony na warstwy.
Odpowiedzialności związane z UI powinny być oddzielone od tzw. logiki biznesowej.
Warstwa widoku od warstwy danych.
Tymczasem u ciebie mamy np. w klasie CartFragment
coś takiego:
val productsInCartList = adapter.cartList
val cartValue = cartViewModel.calculateCartValue(productsInCartList)
To nie jest sensowna architektura.
Adapter jest klasą pomocniczą dla listy, która "żyje" w interfejsie.
Adapter trzyma sobie te dane wyłącznie na użytek mechanizmu UI.
Nie może być traktowany jako "autorytatywne" źródło danych.
To tak, jakby strona internetowa sprawdzała sobie, jaki użytkownik jest zalogowany - szukając tej nazwy w wygenerowanym html-u.
Na zasadzie "a co to ja wcześniej wyświetliłam?".
Sama masz wiedzieć co wyświetliłaś, stara lampucero. Zaciągnij sobie z bazy czy skądś tam.
Interfejs to nie jest notatnik aplikacji, w którym ona ma trzymać sobie własne dane : )
Dane mogą być trzymane w ViewModelu - który z kolei może je pobierać z jeszcze dalszego, niezależnego źródła.
(Byłoby np. logiczne, żeby zawartość koszyka, przynajmniej dla zalogowanego użytkownika, "przeżywała" pomiędzy sesjami. )
A wtedy ViewModel eksponuje te dane w postaci "gotowej do konsumpcji" przez interfejs.
Czyli powinien podawać listę (gotową do wyświetlenia), sumaryczną wartość koszyka (też gotową do wyświetlenia), itd.
Fragment powinien zaś być kompletnie pasywny i ograniczać się do "spięcia" samego siebie z ViewModelem.
W twojej implementacji Fragment "prosi" VIewModel o dokonanie konkretnych przeliczeń, na dodatek podając mu dane.
Czyli w jakimś sensie "rozumie", co wyświetla, a na dodatek zleca przetwarzanie danych.
To jest budowanie domu od komina.
Wszelka logika warunkowa (jak np. "Check if viewed product is already in cart") powinna wylecieć na kopach z Fragmentu na poziom ViewModelu.
Gdybyś pisał testy jednostkowe, ta zasada stałaby się oczywista, bo trudniej jest przetestować jednostkowo logikę we Fragmencie niż w ViewModelu.
We Fragmentach uwagę przykuwają też podejrzane konstrukcje w rodzaju:
viewModel.product.observe(viewLifecycleOwner, {
bindProductData(it)
viewModel.sendProductEvent(it, ProductEventType.DETAIL)
product = viewModel.product.value!! // <- WHAT?
})
Obserwujesz viewModel.product
. Jego wartości dostajesz zatem w lambdzie. To jest owo it
. Dlaczego więc nagle odwołujesz się do viewModel.product.value!!
? To jest, jak to się nieelegancko mówi, wyrywanie zęba przez dupę.
Poczytałbym jeszcze o LiveData
, ViewModelach
itd. Kod, który po części robi coś poprawnie, a po części nie, robi gorsze wrażenie niż konsekwentny błąd.
Wygląda bowiem tak, jakby autor powklejał fragmenty z tutoriali czy StackOverflow bez zrozumienia, a potem dolepiał własną twórczość "na czuja".
##Kotlin
Dawni programiści mawiali: "you can write FORTRAN in every language". Ty piszesz Javę w Kotlinie.
Na przykład null safety nie jest w zasadzie stosowane. Wszędzie widać operator !!
- czyli jeśli wystąpi null, aplikacja po prostu się wywala.
Jedna z ważniejszych przewag Kotlina nad Javą jest zupełnie niewykorzystana.
Operator !!
powinien być stosowany tylko w wyjątkowych, usprawiedliwionych sytuacjach.
On nie przez przypadek wygląda nieestetycznie i alarmująco.
Rozwlekłe konstrukcje rodem ze (starej) Javy też przeniosłeś do Kotlina w skali 1 do 1.
Na przykład:
fun calculateCartValue(list: List<Product>): Long {
var cartValue = 0L
if (list.isNotEmpty()) {
for (product in list) {
cartValue += product.price!!
}
}
return cartValue
}
Te kocie ruchy rękami sygnalizują luz, którego mi najpewniej brakuje, bo ja bym napisał:
fun calculateCartValue(list: List<Product>): Long {
return list.map { it.price!! }.sum()
}
Nie rozumiem przy tym, jaki jest sens warunku if (list.isNotEmpty())
.
Co by się zmieniło, gdyby go nie było?
Taka konstrukcja robi wrażenie, że autor nie wie, jak działa pętla for
(skoro zakłada, że ona sama nie poradzi sobie z iteratorem pustej kolekcji, i trzeba jej podmuchać na łyżeczkę by się nie sparzyła).
W ogóle większość kodu jest rozwlekła i kiepsko napisana.
Na przykład:
private fun checkIfCartIsEmpty(list: ArrayList<Product>) {
if (!list.isNullOrEmpty()) setButtonState(true) else setButtonState(false)
}
-
list
nie może być null, ponieważ typ parametru nie jest nullowalny (jest to ArrayList<Product>
, a nie ArrayList<Product>?
). Sprawdzanie, czy nie zaszła sytuacja, która jest z definicji niemożliwa - mówi mi, że autor nie zna Kotlina.
- Czytelniejsze niż
!list.isNullOrEmpty()
(z podwójnym zaprzeczeniem) byłoby list.isNotEmpty()
. Podwójne zaprzeczenie nie jest zabiegiem nie utrudniającym czytania (see what I did here).
- Czy zamiast
if (list.isNotEmpty()) setButtonState(true) else setButtonState(false)
- nie wystarczyłoby samo setButtonState(list.isNotEmpty())
?.
- Dobra zasada mówi, żeby programować "do interfejsu". Czyli stosować typy możliwie jak najbardziej bazowe, tak aby nie wysyłać fałszywych komunikatów i nie wprowadzać niepotrzebnych ograniczeń. Dlaczego ten parametr musi być akurat typu
ArrayList
, a nie po prostu List
, czy jeszcze lepiej Collection
? Czy funkcja wykorzystuje jakiekolwiek cechy charakterystyczne akurat typu ArrayList
? Nie. O ile dobrze pamiętam, to jest zasada "L" ze słynnego SOLID.
Inny fragment, w którym harmonijnie łączą się wszystkie stylistyczne dewiacje:
// Validate if the required inputs are not empty and correctly formatted
fun validateEmailAndPassword(email: String, password: String): Boolean {
var valid = true
if (TextUtils.isEmpty(email)) {
valid = false
}
if (TextUtils.isEmpty(password) && password.length <= 6) {
valid = false
}
return valid
}
Co sobie myślę jako oceniający.
- Tradycyjnie już nieistotny komentarz (o tym niżej). Co prawda trochę odstaje od większości komentarzy w kodzie. Wartość pozostałych jest zazwyczaj zerowa i tylko zajmują miejsce. Ten jeszcze na dodatek dezorientuje czytelnika, bo poza stwierdzeniem oczywistości - również kłamie; tzn. jego wartość nie jest zerowa, ale nawet ujemna. Ta funkcja wcale przecież nie weryfikuje poprawnego formatu czegokolwiek. (Sprawdza tylko liczbę znaków - zresztą źle).
-
TextUtils.isEmpty(password) && password.length <= 6
- przecież ten warunek jest bez sensu. Jeśli password
będzie mieć np. trzy znaki długości, to warunek nie będzie spełniony, bo wprawdzie password.length
jest wówczas mniejsze niż 6, ale password
i tak nie jest pustym stringiem. Czyli warunek tak naprawdę sprawdza tylko, czy password
jest pustym stringiem (wtedy password.length <=6
przechodzi siłą rzeczy, jako że 0 jest mniejsze niż 6). W żadnym innym wypadku nie będzie już jednak spełniony, a więc liczba 6 jest tu kompletnie nieistotna. Można by ją zastąpić dowolną (nieujemną) wartością, i kod działałby tak samo. Prosty test jednostkowy wychwyciłby tego buga, ale odłożyłeś to na później. (Ja sensu takiej kolejności trochę nie rozumiem - to jakby inżynier oświadczył, że zbuduje most, a w przyszłości zrobi jeszcze obliczenia wytrzymałościowe :) ).
- W Kotlinie nie ma potrzeby używać
TextUtils
. (Z tej samej przyczyny nie tworzyłbym też obiektu Utils
. W Kotlinie można zadeklarować po prostu plik z funkcjami, i tak się przyjęło robić. Sztuczne klasy z cyklu XyzUtils
to proteza stosowana w Javie tylko dlatego, że tam nie da się zadeklarować metod poza klasą).
- Nie wiem, czemu w ogóle używać jakiejś zmiennej
valid
, zamiast po prostu od razu zwrócić false
? Jeżeli wiadomo już, że email był pustym stringiem - czyli jest nieprawidłowy - po co wykonywać dalsze walidacje?
Ja bym tę samą funkcję napisał tak:
fun validateEmailAndPassword(email: String, password: String) =
email.isNotEmpty() && password.length > 6
Oczywiście gdyby płacono mi - jak dziennikarzowi - od wiersza, to wybrałbym pierwszą wersję. Nawet trochę ją podpompował :)
##Komentarze
W zestawieniu z tym co wyżej, to już może drobiazg, ale razi.
A konkretniej - razi mania sadzenia wszędzie bezsensownych komentarzy, które nie dodają żadnej informacji, a tylko powtarzają jak echo to, co widać czarno na białym w kodzie.
Na przykład:
// Handle clicks in the fragment
override fun onClick(view: View?) {
"Handle clicks in the fragment". A co innego może robić metoda onClick
w klasie ProductDetailFragment
?
// Login user
private fun loginUser(email: String, password: String) {
loginUser robi "Login user". Dzięki, stary.
I tak w kółko.
Czasem komentarz jest ambitniejszy i np. próbuje wytłumaczyć nazwę o niejasnym brzmieniu.
// Enable or Disable 'add to cart' button
private fun setButtonState(state: Boolean)
To chwalebny zamiar, ale zamiast nalepiać na kiepską nazwę plaster komentarza, zmieniłbym po prostu nazwę funkcji.
Na przykład:
private fun setAddToCartEnabled(state: Boolean)
albo
private fun enableAddToCartButton(state: Boolean)
I wywalamy komentarz.
Jak zobaczę w kodzie enableAddToCartButton(false)
, rozumiem, co się dzieje.
Jak zobaczę setButtonState(false)
, nie mam pojęcia: jaki button, jaki state.
Muszę iść do implementacji funkcji i przeczytać twój komentarz.
Ale skoro musiałem już przeskoczyć do implementacji, to mogę sobie równie dobrze przeczytać samą funkcję.
Jeżeli komentarze są potrzebne, to jest to zło konieczne.
Najczęściej jest to zło niekonieczne, a więc są niepotrzebne.
Choć bywają (rzadko) niezbędne, sam kod powinien uczynić wszystko, aby były zbędne.
Pisanie komentarzy dla samych komentarzy jest szkodliwym nawykiem, a w niektórych systemach prawnych nawet przestępstwem.