Ocena kodu oraz kilka pytań - Android Kotlin

0

Chciałbym poprosić o ocenę i jakieś wskazówki odnośnie mojego kodu i jednocześnie mam kilka pytań.
https://github.com/Harkor15/ShoppingListNextGeneration
To moja apka - lista zakupów z logowaniem, danymi w firestore i opcją współdzielenie list zakupów po pokazaniu QR kodu przykładowo.

Jestem raczej początkujący w języku Kotlin więc proszę o wyrozumiałość. Wspomniane kilka pytań:

  1. Kotlin umożliwia tworzenie wielu klas w jednym pliku, czy tworzenie sobie tak jak ja to sobie zrobiłem w tej aplikacji klas będących praktycznie POJO w tym samym pliku gdzie są potrzebne, dzięki czemu mogłem szybko zajrzeć do tej klasy jest w porządku? Czy raczej jednak takie klasy powinny być w oddzielnych plikach czy może w jednym pliku z takimi klasami służącymi tylko do przenoszenia danych?

  2. Struktura - Nie mam w tym projekcie sortowania na jakieś package, czytałem już różne opinie na ten temat. Jak to powinno wyglądać? Czy jest sens grupowania tych plików w przypadku gdy jest ich raptem kilka? A jeśli już jakoś tego pilnować to jak? Po prostu klasy Activity razem i tak samo Adaptery itp?

  3. MVVM - Próbowałem już kiedyś jakichś eksperymentów z tym wzorcem, ale czy w przypadku takiej aplikacji miał by on nie wprowadzał by tylko komplikacji? Bo w zasadzie poza jakimś generowaniem QR'ów nie ma tu jakiejś większej logiki aplikacji, więc wydaje mi się że przechodzenie z activity do ViewModelu tylko po to żeby ten wywołał metodę z klasy odpowiedzialnej za komunikacje z bazą danych i później przyjął te dane i oddał z powrotem do warstwy widoku tylko komplikuje niepotrzebnie i wymaga tworzenia jakichś interfejsów i w przypadku niewielkich aplikacji mija się z celem. Czy może po prostu ja jakoś zbytnio komplikuje sobie ten wzorzec?

  4. Testy jednostkowe - Sporo czytałem dobrego słowa o testach i że w ogóle to 100% kodu powinno być testowane, a z drugiej strony trafiłem na okres próbny do działu mobile jednej firmy w której gdy spytałem o testy jednostkowe to usłyszałem coś w stylu "A komu to potrzebne" czy też coś o braku czasu (ostatecznie nie zostałem tam). Ja raczej jestem zwolennikiem pisania testów, bardzo mi pomagały gdy pisałem gierkę na szachownicy i tworzyłem opcje gry z botem. Tylko nie bardzo wiem jak w tej aplikacji która jest tematem tego posta mógłbym ich użyć?

Będę bardzo wdzięczny za feedback odnośnie mojego kodu czy też odpowiedź na choćby jedno z moich pytań lub wskazanie źródła gdzie mógłbym dowiedzieć się czegoś istotnego. Z góry wielkie dzięki.

0

Ustosunkuję się do (4). Z moich obserwacji wynika, że „mobile” jest bardzo niedojrzały, tak jak kiedyś frontend - stąd nie dziwi mnie takie podejście do testów. To trzeba leczyć. Testy to absolutna podstawa (chyba, że robisz pet project, którego nie wdrażasz na proda).

Co do meritum - testuj jednostkowo logikę aplikacji. W jakimś celu ją zbudowałeś, realizuje jakieś konkretne zadania, coś oblicza itp. Spróbuj to przetestować w oderwaniu od faktu, że jest to odpalane na Androidzie. Jeśli napotkasz problemy, bo jesteś zależny od SDK, to znaczy, że powinieneś poprawić design aplikacji, aby była ona testowalna jednostkowo.

0
Charles_Ray napisał(a):

Ustosunkuję się do (4). Z moich obserwacji wynika, że „mobile” jest bardzo niedojrzały, tak jak kiedyś frontend - stąd nie dziwi mnie takie podejście do testów. To trzeba leczyć. Testy to absolutna podstawa (chyba, że robisz pet project, którego nie wdrażasz na proda).

Co do meritum - testuj jednostkowo logikę aplikacji. W jakimś celu ją zbudowałeś, realizuje jakieś konkretne zadania, coś oblicza itp. Spróbuj to przetestować w oderwaniu od faktu, że jest to odpalane na Androidzie. Jeśli napotkasz problemy, bo jesteś zależny od SDK, to znaczy, że powinieneś poprawić design aplikacji, aby była ona testowalna jednostkowo.

Dzięki za odpowiedź, tak jak pisałem wcześniej korzystałem z testów jednostkowych przy aplikacji która miała dość skomplikowaną logikę. Tylko nie bardzo widzę zastosowanie testów w aplikacji która w zasadzie nie bardzo ma jakąś logikę. W pytaniu chodziło mi właśnie o sens zastosowania testów jednostkowych w aplikacji która de facto tylko odbiera dane od użytkownika i wrzuca je do bazy danych i odwrotnie: pobiera dane i wyświetla. Co tu testować?
Na siłę może by się coś banalnego znalazło tylko czy to nie będzie sztuka dla sztuki?
To apka o której mowa: Link

0

Jeśli nie ma logiki, to przetestowałbym to tylko integracyjnie. Natomiast w kodzie widzę jakieś ify, pętle itp - skąd masz pewność, że to działa? Dlaczego sprawdzanie warunków „biznesowych” jest w komponentach Androidowych? Obecnie nie widzę ani jednego testu, to niezbyt dobre ekstremum.

0

Jak to kiedyś powiedziano, "you can write FORTRAN in any language".

Kod jest niby w Kotlinie, ale pisany tak, jakby to była Java. A zresztą nawet w Javie byłby dość kulawy. Pokażę to na przykładzie:

    private fun addSharedList(code: String) {
        val userAndListId = code.split("_")
        if (userAndListId.size == 2) {
            var duplicateCheccker = false
            if (adapter.sharedItems.size != 0) {
                for (i in 0 until adapter.sharedItems.size) {
                    if (adapter.sharedItems[i].listId == userAndListId[1]) {
                        Toast.makeText(this, R.string.this_list_is_already_added, Toast.LENGTH_SHORT).show()
                        duplicateCheccker = true
                    }
                }
            }
            if (!duplicateCheccker) {
                firestoreManager.addSharedLists(userAndListId[0], userAndListId[1], this, this)
            }
        }
    }

Pomińmy już literówkę (checcker) i kiepskie nazewnictwo ("checker" to byłaby raczej jakaś klasa, reprezentująca proces wyszukiwania duplikatów - wartość sygnalizującą ich obecność nazwałbym raczej duplicatesDetected, albo coś w tym stylu).

W tym kodzie wiele rzeczy jest bez sensu. Np. jaki jest sens warunku:

if (adapter.sharedItems.size != 0) {
    for (i in 0 until adapter.sharedItems.size) {

?

Już samo != 0 wygląda dziwnie, bo przecież size nie może być mniejsze od zera, więc naturalniej byłoby napisać size > 0. Ale czemu w ogóle ten warunek służy? Przecież jeśli ta kolekcja jest pusta (size == 0), pętla for się po prostu nie wykona. Dodatkowe zabezpieczenie się przed taką sytuacją jest zbędne.

A skoro piszemy w Kotlinie, ten rozwlekle obkodowany algorytm ująłbym prościej, i chyba czytelniej. Na przykład tak:

    private fun addSharedList(code: String) {
        val userAndListId = code.split("_")
        if (userAndListId.size != 2) {
            return
        }
        val duplicatesDetected = adapter.sharedItems.any {
            it.listId == userAndListId[1]
        }
        if (duplicatesDetected) {
            Toast.makeText(this, R.string.this_list_is_already_added, Toast.LENGTH_SHORT).show()
        } else {
            firestoreManager.addSharedLists(userAndListId[0], userAndListId[1], this, this)
        }
    }

Bez niepotrzebnych pętli.

Kłuje mnie swoją drogą w oczy nazwa zmiennej "userAndListId". Najbardziej koszernie byłoby, żeby ta para wartości była reprezentowana przez jakąś małą klasę (data class), z dwiema zmiennymi, a nie przez generyczny typ (lista), gdzie wartości wyciągamy poprzez magiczne numerki, o których przypomina nam hybrydowa nazwa.

Ale nawet jeśli nie dokładać dodatkowej klasy, wciąż można sobie darować te szpetne userAndListId[1], na przykład wywalając parsowanie do osobnej metody. Dajmy na to:

    private fun String.parse(): Pair<String, String>? = split("_")
        .takeIf {
            it.size == 2
        }
        ?.let {
            it.first() to it.last()
        }

    private fun addSharedList(code: String) {
        val (userId, listId) = code.parse() ?: return
        val duplicatesDetected = adapter.sharedItems.any {
            it.listId == listId
        }
        if (duplicatesDetected) {
            Toast.makeText(this, R.string.this_list_is_already_added, Toast.LENGTH_SHORT).show()
        } else {
            firestoreManager.addSharedLists(userId, listId, this, this)
        }
    }

1 użytkowników online, w tym zalogowanych: 0, gości: 1