Android - ocena projektu na Githubie

Cześć wszystkim,

zdaje sobie sprawę, że takich postów pojawia się tutaj sporo. Ale mam nadzieję, że nie zostanę zlinczowany :) Mianowicie szukam od jakiegoś czasu pracy jako ten nieszczęsny Junior. Dokładniej jako Junior Android Developer. I niedawno skończyłem jeden z większych swoich projektów, który wrzuciłem na Github'a. I tutaj chciałbym was poprosić o rzucenie okiem i taką konstruktywną opinię. Czy idę w dobrą stronę czy jeszcze sporo przede mną przed pierwszą pracą :)

https://github.com/voximodoo/fridify

Z góry dziękuje wszystkim.

PS. I nie, nie jestem po żadnym campie, mam za sobą normalnie skończoną uczelnię o jakiejś tam w miarę renomie :)

1. Usuń folder .idea z repozytorium i dodaj ten folder do .gitignore. Masz tam konfigurację swojego IDE, które może być inna niż mojego. W teorii mógłbym nie korzystać z IntelliJ'a / Android Studio, więc tym bardziej nie powinno być tego folderu.

2. Patrzę w historię commitów i dostaję informację, że źle korzystasz z narzędzia. Jeżeli migrowałeś z Bitbucketa na GitHuba, to trzeba było przemigrować razem z historią i nie robić z tego jednego commita. Drugi commit jest ogromny Showing 89 changed files with 2,428 additions and 349 deletions. W pracy byłoby to nie do ogarnięcia dla osoby przeglądającej kod.

3. Dodane zależności i pluginy w Gradlu, z których w ogóle nie korzystasz. Kotlin i biblioteki do testowania do wywalenia razem z plikami ExampleUnitTest.java i ExampleInstrumentedTest.java. Przez źle dodanego Kotlina projekt się nawet nie będzie budował. Wszystkie zależności masz dodane z implementation a jedną niepotrzebnie compile - compile 'com.android.support:support-v4:27.1.1'.

4. Dodajesz reguły dla ProGuarda, ale w ogóle z niego nie korzystasz. Albo wywal reguły i konfigurację w Gradlu, albo go włącz.

5. Nazwy pakietów w Javie powinny być z małej litery. Przy okazji lepiej jest dzielić klasy w pakietach per feature zamiast per warstwa. W małych aplikacjach per warstwa jest może trochę łatwiej, ale im dalej w las tym trudniej. Tutaj możesz przeczytać trochę więcej o tym.

6. Niekosekwentnie nazywasz zmienne. Raz z notacją pseudowęgierską, raz po ludzki. Powinieneś mieć jednolity styl. I najlepiej nie korzystać z dziwacznych przedrostków - https://jakewharton.com/just-say-no-to-hungarian-notation.

7. Klasa GLOBAL_FLAGS kojarzy mi się z jakimiś propertisami z rozwiązań backendowych. Tak średnio. Po pierwsze nazwy tych stałych są mylące, bo np. coś nazywa się NOTIFICATION_TIME_BEFORE_EXPIRING a służy za klucz w SharedPreferences. Po drugie takie klasy mają tendencję do pałętania się wszędzie i zanieczyszczania kodu.

8. Jak już jestem przy SharedPreferences to mógłbyś zrobić z nich singletona tak jak w przypadku AppDatabase. Tak naprawdę nie powinno się tworzyć takich singletnów w ogóle, ale w Androidzie ciężko tego uniknąć na początku (aczkolwiek można).

9. Pakiet Models jest ogólnie bardzo dziwny. Ciężko mi zrozumieć czym ten pakiet ma się zajmować. Masz tu praktycznie wszystko - bazę danych, view modele, jakaś klasa ProductUI i DAO dla niej. View modele nie powinny znajdować się w tych pakietach na pewno. Mają też niejednolite nazwy. Raz CośtamModelView (dosyć niekonwencjonalna kolejność w nazwie), raz CośtamViewModel.

10. View modele dziedziczą po AndroidViewModel tylko po to, żeby dostać Application i stworzyć "repozytoria". To prowadzi do tego, że praktycznie każda warstwa w aplikacji zależy od Androidowego frameworka i nie da się tego łatwo testować. Zamiast przekazywać Application powinieneś przekazywać im interfejsy, których faktycznie potrzebują. Same view modele powinny być tworzone za pomocą jakiejś własnej fabryki, bo akurat tego wymaga biblioteka.

11. Kiedy szukasz widoków za pomocą findViewById raz wynik rzutujesz innym razem nie. Nie powinieneś rzutować w ogóle, bo od jakiegoś czasu metoda jest generyczna i nie wymaga rzutowania.

12. Twoje Activity robią dosyć sporo. Zajmują się UI, trochę logiką, trochę rozmawianiem z frameworkiem. Powineneś porozbijać to na mniejsze klasy, które miałby konkretne odpowiedzialności.

13. Trochę niepotrzebnego albo źle poframotwanego kodu. Nieużywane zmienne, niepotrzebne metody, niepotrzebnie nadpisywane metody itp.

14. Nie korzystaj z formatu Serializable. Jeśli musisz coś przekazywać poprzez Intent jako cały obiekt, używaj Parcelable.

15. Takie warunki foundProduct.getDaoState() == Product.exisitingNotChange && !cbModifyProduct.isChecked() się cięzko czyta. Po pierwsze czym w ogóle jest daoState? Po drugie jak modelujesz skończone stany używaj do tego enumów. Po trzecie warunki powinny być wyniesione do osobnych metod dla czytelności.

16. Przy niektórych AsyncTask masz potencjalne wycieki pamięci. Np. te w ActivitySettings.

17. Bonus - wszędobylskie gettery i settery.

Uwag trochę wypisałem i pewnie znalazłbym więcej, gdybym szczegółowo przejrzał kod. Duże znaczenie ma czytelność - formatowanie, nazewnictwo, itd. Niemniej uważam że będą z Ciebie ludzie. :)

Michał Sikora napisał(a):

1. Usuń folder .idea z repozytorium i dodaj ten folder do .gitignore. Masz tam konfigurację swojego IDE, które może być inna niż mojego. W teorii mógłbym nie korzystać z IntelliJ'a / Android Studio, więc tym bardziej nie powinno być tego folderu.

Akurat w ramach .idea jest kilka rzeczy które warto sobie dzielić w zespole (konfiguracje formatterów, inspekcji, połączenia z bazami, typowe run configi np do debugu), tutaj można więcej poczytać:

https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems

Nie bez powodu czasami są ustawienia globalne a czasami na cały projekt, podobnie ten checkbox Share w run configach też po coś jest. Niestety pod tym względem IntelliJ jest niedorobiony (np od lat nie mogą ogarnąć dzielenia słowników). Kwestia oczywiście obgadania tego w zespole, ale jak 90% osób używa IntelliJa to chyba jest to sensowne.

A gdzie masz testy ?