Hej!
Pierwszy większy projekt, aplikacja ecommercowa korzystająca z Firebase, w jakimś stopniu starałem się zachowywać zasady architektury MVVM, bardzo proszę o ocene bo od tego chciałbym wyjść co dalej, na czym się skupić i co jest do poprawy.
Aplikacja nie jest skończona, ale moje podstawowe założenia spełnia i pisało mi się ją wybitnie przyjemnie.
Kod: https://github.com/Kerubyte/EngageCommerce
Dzięki!
Czemu pliki z Kotlina są w src/main/java?
Tak się nie robi w gradlu/mavenie ani w ogóle. Wzorzec jest prosty: src/main/$nazwaJęzyka
Pierwsza rzecz, którą sprawdzam w każdym projekcie (nie jestem w tym odosobniony): czy są testy jednostkowe, i jak wyglądają. U ciebie wyglądają tak, że ich nie ma. To znaczy zostały na swoim miejscu ExampleUnitTest i ExampleInstrumentedTest, co w moim odbiorze dodaje posmaku szyderstwa : )
Druga rzecz, którą sprawdzam, to historia commitów. Czy autor potrafi posługiwać się systemem kontroli wersji? Czy z historii wynika, że umie zorganizować sobie pracę? Czy nadaje commitom sensowne, czytelne nazwy?
U ciebie commity nazywają się* "Initial commit"* (w porządku), a potem: "added several features", "added several features". A, i wreszcie "added several features". Jakie wywiera to na sprawdzającym wrażenie, pozostawię domyślności autora : ) Może pomyśleć, że ma do czynienia z fanem Gombrowicza ("Poniedziałek - Ja, Wtorek - Ja, Środa - Ja..."), ale może i co innego pomyśleć.
Przejrzę później sam kod, ale gdyby ktoś robił to w procesie rekrutacji - zwłaszcza mając większą liczbę kandydatów do oceny - to nie mógłbyś być zdziwiony, gdyby na tym etapie otworzył już sobie projekt następnego kandydata.
@V-2: Dziękuje! Najbardziej mi zależny na ocenie mojego kodu i jak to generalnie wygląda dla osoby z bardzo małym doświadczeniem. Do pracy jako programista na pewno się nie wybieram w tym roku, dużo nauki przede mną z 'inzynierii' oprogramowania i mam mnóstwo tematów do ogarnięcia jeśli chodzi o konfiguracje projektu i teoretyczne podstawy.
Pierwsza rzecz, którą sprawdzam w każdym projekcie (nie jestem w tym odosobniony): czy są testy jednostkowe, i jak wyglądają. U ciebie wyglądają tak, że ich nie ma. To znaczy zostały na swoim miejscu ExampleUnitTest i ExampleInstrumentedTest, co w moim odbiorze dodaje posmaku szyderstwa : )
Druga rzecz, którą sprawdzam, to historia commitów. Czy autor potrafi posługiwać się systemem kontroli wersji? Czy z historii wynika, że umie zorganizować sobie pracę? Czy nadaje commitom sensowne, czytelne nazwy?
Added several features
- added full checkout proccess
- added basic dark mode handling
- numerous changes to UI
- redesign of profile fragment
- redesign of navigation drawer
- small bugfixes
Commity o nazwach 'added features' nie musisz tłumaczyć jakie wrażenie wywierają :D
Przejrzę później sam kod, ale gdyby ktoś robił to w procesie rekrutacji - zwłaszcza mając większą liczbę kandydatów do oceny - to nie mógłbyś być zdziwiony, gdyby na tym etapie otworzył już sobie projekt następnego kandydata.
Kerubyte napisał(a):
Nazwy poszczególnych commitów w android studio są rozbudowane bardziej:
Added several features
- added full checkout proccess
- added basic dark mode handling
- numerous changes to UI
- redesign of profile fragment
- redesign of navigation drawer
- small bugfixes
W wielu programach klienckich, tak samo jak na liście na GitHubie, domyślny podgląd obejmuje tylko pierwszą linię commitu.
Powinna więc być czytelniejsza, a w kolejnych liniach to można ewentualnie doprecyzować drugorzędne szczegóły.
Nie powinna jednak robić się z nich taka "lista zakupów".
"Full checkout process" czy "dark mode handling" to są prawdopodobnie zmiany niezwiązane ze sobą, Powinny więc stanowić osobne commity.
Rozumiem jaki jest odbiór i biorę to pod uwagę od razu ale w tym projekcie chodziło o spełnienie założeń aplikacji, napisanie jest w większości 'samemu' i w czegoś takiego na potencjalnej rekrutacji bym w nie pokazał.
Rozumiem, aczkolwiek (moim zdaniem) nawet jeśli pisze się tylko dla siebie, warto wyrabiać sobie dobre nawyki. Zresztą nawet kiedy nie pokazujesz pracy nikomu z zewnątrz, to czytelna struktura historii i tak może się przydać. Wystarczy, że projekt urośnie do rozmiaru, w którym coś się zacznie plątać - a trudno zawczasu przewidzieć, kiedy osiągniemy ten rozmiar - i nagle docenimy, że łatwo nam znaleźć konkretny moment z przeszłości, do którego chcielibyśmy zajrzeć.
Dzięki, jeśli znajdziesz czas, będe wdzięczny za dalsze uwagi.
Jasne, postaram się - chyba, że inni użytkownicy łapczywie zdążą już wszystko powytykać i zostawią same kości
@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.!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).if (list.isNotEmpty()) setButtonState(true) else setButtonState(false) - nie wystarczyłoby samo setButtonState(list.isNotEmpty())?.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.
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 :) ).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ą).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.
@V-2:
Dziękuje. Widziałem po innych tematach jak podchodzisz do Code Review i dziękuje, że miałeś czas aby zajrzeć i podzielić się wiedzą. Jestem naprawdę wdzięczny.
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.
Trochę zbyt na szybko podszedłem do commita, zmieniłem kilka rzeczy w różnych miejscach, to jest generalnie problem, że skaczę od klasy do klasy i coś tam dodłubuje. I takie są tego efekty, zmieniam to właśnie starając się zrobić rzeczy po kolei. W tym commicie faktycznie przeniosłem timeNow z repo do utils a potem w ogole wydzieliłem go do podawania w ViewModel.
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.
Zgadzam się, nawet nie ma o czym mówić. Mój kod jest pełen workaroundów, które spinają się na aplikacje działającą tak jak sobie założyłem ale wykonanie tego jest takie jak widzisz, stosowanie w prawie każdej klasie jakiś anty-patternów.
Na tym przykładzie problem jest taki:
Na początku było mniej więcej tak jak mówisz, userCart, cartValue oraz cartSize były zaciągane z repo w View Modelu i podawane do fragmentu jako gotowe dane ale miało to jedną poważną dla mnie konsekwencje, po usunięciu produktu z koszyka dane takie jak ilość produktów czy wartość koszyka pozostawała na stałym poziomie, musiałem wejść do fragmentu ponownie aby dane się zaktualizowały. Starałem się na różne sposoby czytać w docsach oraz na stack overflow ale nie potrafiłem znaleźć jak to ogarnąć. W innym fragmencie używam Snapshot Listenera ale tutaj jakoś mi on nie pasował. Nie ukrywam, że jeśli ktoś z czytających mógłby mnie nakierować na dokumentacje, która pokaże mi jak taki problem rozwiązać to będzie mi to bardzo pomocne.
Załączam wideo jak działa aplikacja i te kulawe ekwilibrystyki w kodzie są moją reakcją na początkową trudność w zrobieniu takiej funkcjonalości jak potrzebuje tak jak się powinno.
Plik screen-20210308-203001.7z
Kolejna kwestia, tego typu funkcja jest w porzadku czy powinienem to obsłużyć w inny sposób?
override fun onProductClick(product: Product, position: Int) {
cartViewModel.removeFromCart(product)
adapter.removeFromCart(product, position)
}
Tutaj prosze o coś View Model ale muszę w jakiś sposób wywołać funkcje z VM i podać co dokładnie chcę usunąć, VM nie wie w jaki produkt kliknąłem przecież.
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ę.
O matko, racja, kompletnie to przeoczyłem, masz racje, w miare rozumiem skąd się bierze it w lambdzie :D W miare.
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())?.
Czytelność kodu i jego klarowność na razie leży jak widzę, absolutnie ma dla mnie sens to co piszesz i faktycznie widzę, że podczas pisania robie rzeczy na czuja lub tak jak IDE podpowiada zamiast samemu mieć nad tym kontrole i głębiej rozumieć co pisze, cenna lekcja.
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.
Na tego typu kwestie jeszcze nie zwracałem uwagi, dzięki bo to troche otworzyło mi spojrzenie na wiele innych miejsc w kodzie, które mogłem w myśl SOLID kompletnie inaczej rozwiązać lub napisałem je bez sensu, "bo działają".
Inny fragment, w którym harmonijnie łączą się wszystkie stylistyczne dewiacje:
Taak hehe, z tymi niby prostymi walidacjami sam sie z siebie śmieje trochę, tutaj wiem co pokpiłem, zakodowałem żeby było, potem w jakieś wolnej chwili wydzieliłem je do Utils i porzeźbiłem, przy tym akurat wiedziałem, ze jest to do zmiany/wywalenia asap. Nic jednak z tym nie zrobiłem niestety..
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.
Tak, komentarze już są usunięte, nazwy funkcji przemyśle lepiej aby kod był samooopisowy jeśli to będzie możliwe. Naklepałem te komentarze pare miesięcy temu i tak zostały, w nowszych fragmentach już ich nie stosowałem starając się pisać kod, który będzie łatwy do analizy i ja jak wroce kiedyś to zobacze o co tam chodziło.
Dobrze, ja w wolnych chwilach pracuje nad tą aplikacją i postaram się pracować teraz inaczej bo byłem zadowolony z tej aplikacji(działania) ale to co pod spodem widze teraz że jest napisane bez głowy. Jak ktoś ma materiały pisane, które uważa za wartościowe do nauki to śmiało dajcie znać gdzie jest zdobyć :)
Dzięki!
EDIT
Spójrzcie proszę na zmiany w ProductDetailsFragment i jego ViewModel, czy tak to bardziej się łapie na separation of concerns?
Kilka uwag do kolejnych zmian.
Takie coś nie powinno się dziać we Fragmencie:
viewModel.createOrderFromCart()
viewModel.clearUserCart()
Ten Fragment jest nadal "mądry", bo dyryguje ViewModelem.
Np. wie, że te metody powinien wywołać w akurat takiej kolejności.
ViewModel powinien wystawić jedną metodę, gotową do użytku. I to w niej powinna się zamykać wszelka "mądrość".
Czyli np.
viewModel.placeOrder()
I tyle. Stworzenie zamówienia, wyczyszczenie koszyka itd. to są szczegóły, które nie powinny interesować Fragmentu.
Nawiasem mówiąc, samą obsługę onClick też załatwiłbym databindingiem.
To samo tutaj:
viewModel.productInCart.observe(viewLifecycleOwner, {
enableAddToCartButton(it)
A czemu nie powiązać atrybutu tego buttona bezpośrednio z viewModelem w xml-u?
android:text="@{viewModel.currentProduct.price.toString()}"
Drobiazg, ale i ten toString() też przesunąłbym do ViewModelu.
Najlepiej, by ViewModel eksponował dane w 100% gotowe do "konsumpcji".
override fun onStop() {
super.onStop()
viewModel.snapshotListenerRegistration?.remove()
}
To jest kiks. Takie rzeczy powinien robić sam viewModel, w onCleared().
W tej chwili ten ViewModel jest troszkę niepełnosprawny i Fragment musi mu podcierać pupę.
Gdybym chciał użyć tego samego ViewModelu w jakimś Activity, to musiałbym wiedzieć i pamiętać, że w tym Activity też muszę ręcznie oprogramować usuwanie listenera.
To jest błąd implementacyjny.
Log.d("snapshotProduct", it.message.toString())
Nie logowałbym wyjątku jako it.message.toString(). Traci się w ten sposób resztę informacji o wyjątku (np. ślad stosu).
Błąd logowałbym na wyższym poziomie, np, Log.w czy Log.e, a wtedy dostępne są przeciążenia, które akceptują sam obiekt Throwable.
Czyli wystarczy:
Log.w("snapshotProduct", it)
To kosmetyka, ale starałbym się pisać bardziej idiomatycznie.
Zamiast:
val myParams: HashMap<String, Any> = HashMap()
myParams["name"] = currentProduct.value?.name.toString()
myParams["price"] = currentProduct.value?.price.toString()
myParams["Image_URL"] = currentProduct.value?.imageUrl.toString()
Wystarczyłoby:
val parameters = mapOf(
"name" to currentProduct.value?.name.toString(),
"price" to currentProduct.value?.price.toString(),
"Image_URL" to currentProduct.value?.imageUrl.toString()
)
Nie nazywałbym wartości myParameters, bo nie bardzo rozumiem, co to ma podkreślać ;) Inne mają innego właściciela? Są pożyczone? Czemu nie myRepository, myProductUid itd.? :)
Co ważniejsze, mam wątpliwości co do twojego notorycznego stosowania metody rozszerzającej toString(), żeby przekonwertować wartość String? do String.
Jeśli currentProduct.value będzie nullem, to w parametrach pójdą wtedy dosłownie wartości "null" (tj. czteroliterowy napis "null") - jako nazwa, jako cena, jako url do obrazka. Czy na pewno tego chcemy??
@V-2: Hej!
Ten Fragment jest nadal "mądry", bo dyryguje ViewModelem.
Np. wie, że te metody powinien wywołać w akurat takiej kolejności.
Nawiasem mówiąc, samą obsługę onClick też załatwiłbym databindingiem.
Tak, tutaj już sobie to dziś zmieniłem, restartMainActivity() obsługuję poprzez LiveData.
A czemu nie powiązać atrybutu tego buttona bezpośrednio z viewModelem w xml-u?
Hmmm, mówisz żeby wystawić to LiveData do xmla? Królu złoty, rewelacja, dzięki za ten tip! android:enabled="@viewModel.productInCart"
Drobiazg, ale i ten toString() też przesunąłbym do ViewModelu
Jasne, tutaj wydawało mi się, że 'czyściej' będzie w ViewModelu z jednym LiveData i resztę danych powyciągam w xmlu
To jest kiks. Takie rzeczy powinien robić sam viewModel, w onCleared().
Muszę poczytać :/ To głupi błąd wynikający z niedokładnej wiedzy podstaw ViewModeli.
Nie nazywałbym wartości myParameters, bo nie bardzo rozumiem, co to ma podkreślać ;) Inne mają innego właściciela? Są pożyczone? Czemu nie myRepository, myProductUid itd.? :)
Co ważniejsze, mam wątpliwości co do twojego notorycznego stosowania metody rozszerzającej toString(), żeby przekonwertować wartość String? do String.
To akurat skopiowane z dokumentacji sdk, toString dałem bo krzyczało mi błąd, tutaj nie analizowałem niczego tak na prawdę, działa to działa. W innych miejscach napisanych przeze mnie sprawdze czy nie mam takich baboli.
Generalnie dobrze wiem co i jak napisać(poniekąd) ale brak mi wiedzy w jaki sposób rozwiązać problemy, polecasz coś do nauki programowania Androida(do Kotlina mam książke) oprócz dokumentacji i artykulow na blogach/medium?
Dzięęki! Ogromną ilość wiedzy przyswoiłem po Twoim CR w ciągu ostatniego tygodnia, jestem fanem code review od teraz hehe
Edit
polecasz coś do nauki programowania Androida(do Kotlina mam książke) oprócz dokumentacji i artykulow na blogach/medium?
Heh, nie odkrywając ameryki widzę że praktycznie wszystkie informacje, które mi przekazujesz są zawarte w dokumentacji, nevermind
Kerubyte napisał(a):
Drobiazg, ale i ten toString() też przesunąłbym do ViewModelu
Jasne, tutaj wydawało mi się, że 'czyściej' będzie w ViewModelu z jednym LiveData i resztę danych powyciągam w xmlu
Są dwie szkoły myślenia. Formatowanie można uznać za odpowiedzialność widoku (czyli wpakować do xml-a). Taką samą, jak np. dobór kolorów. Idea jest taka, że np. dzięki temu tego samego ViewModelu można użyć w dwóch różnych widokach (które mogłyby stosować inny format), a sam ViewModel "nie wie" - bo nie powinien - z którym jest spinany. Z drugiej strony takiego xml-a nie da się przetestować jednostkowo, więc pakowanie do niego logiki (nawet najprostszej) też ma swoje minusy.
Generalnie dobrze wiem co i jak napisać(poniekąd) ale brak mi wiedzy w jaki sposób rozwiązać problemy, polecasz coś do nauki programowania Androida(do Kotlina mam książke) oprócz dokumentacji i artykulow na blogach/medium?
Czytaj Android Weekly (przy okazji można i Kotlin Weekly), poszukałbym też na GitHubie dobrych projektów open-source i sobie je postudiował. Zwłaszcza "pokazowe", które proponują jak może wyglądać sensowna architektura. Np. https://github.com/android/sunflower
Żadnych takich projektów bym jako prawdy objawionej nie traktował, lepiej też sprawdzać, czy projekt nie ma np. 5 lat, bo może zawierać nieświeże pomysły. Ale zawsze to jakaś inspiracja.
No i skupiłbym się na testach jednostkowych, bardzo zmieniają podejście do programowania, bez tego trudno wejść na wyższy poziom.
W aplikacji bardzo dużo się zmieniło i w końcu pewnie ją napisze od nowa żeby architektura była odpowiednia ale efekty mi się podobają, jak ktoś ma chwilę, na przykład @V-2 to śmiało :)
@Kerubyte: Od siebie dodam jedynie, że do każdego projektu (który wystawiamy ocenie, lub celowo publikujemy) fajnie jest dodać README, które wprowadzi przeglądającego w kod, sam projekt, jego ideę, cel, może pokrótce przedstawi architekturę, lub zaprezentuje użycie/rezultat.
Mam prośbę, czy mógłby ktoś zerknąć na klasę repozytorium oraz jej połączenie z viewModelem?
To działa tak jak bym sobie tego życzył ale wydaje mi się, że źle to napisałem i logicznie powinno to być inaczej rozwiązane.
W repo mam metodę, która pobiera mi zawartość koszyka użytkownika getUserCart oraz liveData, które trzyma stan koszyka. getUserCart zmienia wartość liveData w repo po wywołaniu. viewModel obserwuje(nie usuwam obserwatora jeszcze w onCleared, to testy) to liveData i przekazuje informacje o stanie do widoku poprzez dataBinding.
Problemy, które staram się rozwiązać to dynamiczna zmiana widoku po usunięciu produktów z koszyka, korzystam z sealed class UserCartState aby obsłużyć zmiany w widoku dla koszyka puste, z zawartością oraz błędu.
Repo ->
class UserRepository
@Inject constructor(
private val firestore: FirebaseFirestore,
private val auth: FirebaseAuth,
private val inputUserMapper: NullableInputUserEntityMapper,
private val outputUserMapper: NullableOutputUserEntityMapper
) : UserDatabase {
private val _userCartState: MutableLiveData<UserCartState.UserCart> = MutableLiveData()
val userCartState: LiveData<UserCartState.UserCart>
get() = _userCartState
override fun getUserCart() {
auth.currentUser?.let { firebaseUser ->
val uid = firebaseUser.uid
firestore.collection(COLLECTION_USERS)
.document(uid)
.get()
.addOnSuccessListener {
val userEntity = it.toObject(UserEntity::class.java)
val user = inputUserMapper.mapFromEntity(userEntity)
if (user.cart.isNullOrEmpty()) {
_userCartState.value = UserCartState.UserCart.Empty
} else {
_userCartState.value = UserCartState.UserCart.NotEmpty(user.cart)
}
}
.addOnFailureListener {
_userCartState.value = UserCartState.UserCart.Error
}
}
}
}
viewModel
@HiltViewModel
class CartFragmentViewModel
@Inject
constructor(
private val userRepository: UserRepository,
private val priceFormatter: PriceFormatter
) : ViewModel() {
userRepository.userCartState.observeForever {
when (it) {
is UserCartState.UserCart.Empty -> handleEmptyCart()
is UserCartState.UserCart.NotEmpty -> handlePopulatedCart(it.list)
is UserCartState.UserCart.Error -> handleCartError()
}
}
private fun getUserCart() {
userRepository.getUserCart()
}
init {
getUserCart()
}
}