Hej, chciałbym poprosić o code review mojego projektu:
https://github.com/Hobbajt/BubbleQuiz
Gra polega na "mizianiu" palcem i rozdrabnianiu bąbelków na mniejsze, a następnie odgadnięciu, co prezentuje zdjęcie.
Będę wdzięczny za każdą radę i wskazówkę
Projekt jest na tyle duży, że porządny code review wymagałby sporo pracy. Przejrzałem go tylko na wyrywki, tak że uwagi będą na chybił trafił.
Kod jest ogólnie dość spoko. Co bym poprawił albo co mi się subiektywnie nie podoba:
Architektura
BuildConfig w skrypcie Gradle'a, ewentualnie jako resource tekstowy. Gdybyśmy chcieli mockować API (na potrzeby testów, których nie ma), to i tak mockowalibyśmy raczej obiekt, który do tego API się podłącza - taki jak LevelPacksApiLoader, który w wersji testowej zwracałby jakieś dane ustawione na sztywno, zamiast pobieranych z netu... DI to jest narzędzie do rozwiązywania pewnego problemu, a nie cel sam w sobie ;)Kotlin
Przeciążanie equals i hashCode w obiekcie data class (jak BubblesState) nie powinno być w ogóle potrzebne. To się mija z ideą data class, która zapewnia taką implementację automatycznie. A kolekcje Kotlinowe są porównywane po wartościach! Nie musimy o to dbać "ręcznie".
drawItem.position.x == positionPx.x && drawItem.position.y == positionPx.y
Wystarczyłoby drawItem.position == positionPx, bo Position to jest data class, czyli są porównywane po wartościach... doczytaj, czym są data classes, bo z kodu przebija w kilku miejscach przekonanie, że niczym nie różnią się od zwykłych ;) (Co pozwala się zastanawiać, czemu autor w ogóle stawia to data w deklaracjach).
W Kotlinie nie trzeba używać .equals do porównywania Stringów (co robisz np. w AnswerChecker).
Nie ma potrzeby używać StringUtils (zamiast StringUtils.isNotBlank(answer) w InputViewContainer po prostu answer.isNotBlank()).
Podobnie nie ma potrzeby używać ObjectUtils.defaultIfNull<Set<Int>>(passedLevels, HashSet()), w każdym razie mi się wydaje, że passedLevels ?: emptySet() wyglądałoby lepiej. To jedna z różnic między Kotlinem a Javą.
constructor(context: Context) : super(context)
constructor(context: Context, attrs: AttributeSet) : super(context, attrs)
constructor(context: Context, attrs: AttributeSet, defStyleAttr: Int) : super(context, attrs, defStyleAttr)
Idiomatyczną alternatywą w Kotlinie jest @JvmOverloads. Zob. https://antonioleiva.com/custom-views-android-kotlin/
To jest zasadniczo bez sensu:
class BitmapUtilities
{
companion object
{
fun convertBitmapToByteArray(bitmap: Bitmap): ByteArray
// ciach
}
Skoro cała implementacja to jest companion object (czyli statyki), można by to zastąpić deklaracją object BitmapUtilities, i wywalić dodatkowe zagnieżdżenie companion object. Natomiast najlepiej byłoby wywalić całą w ogóle klasę. Klasy typu ...Utils to jest zmora Javy, klasy tworzone tylko po to, żeby były wiadrami na funkcje. W Javie trudno uciec od tego wzorca, bo nie pozwala ona na implementację funkcji/metod poza obrębem klas. W Kotlinie natomiast można to robić, i jest to rozwiązanie idiomatyczne. Plik może zawierać po prostu zestaw funkcji użytkowych, nieprzypisanych do żadnej sztucznie nazywanej klasy. Tak są napisane np. biblioteki standardowe Kotlina. Np. filter to jest funkcja rozszerzająca w pliku _Collections.kt, a nie w jakiejś klasie CollectionUtilities. object przydaje się wtedy, gdy coś jest singletonem, ale zawiera jednak pewien stan. W przypadku funkcji pomocniczych żaden obiekt nie jest potrzebny.
Rozwlekła nazwa tej funkcji aż się prosi, by zaimplementować ją jako funkcję rozszerzającą.
Tak więc zamiast
fun convertBitmapToByteArray(bitmap: Bitmap): ByteArray
Mielibyśmy:
fun Bitmap.convertToByteArray(): ByteArray
Czy w zasadzie nawet:
fun Bitmap.toByteArray(): ByteArray
bo "convert" rozumie się samo przez się (convertToString, anyone?), i wtedy w kodzie zamiast:
listener.onSuccess(BitmapUtilities.convertBitmapToByteArray(loadedImage))
Wystarczyłoby:
listener.onSuccess(loadedImage.toByteArray())
Kwestia gustu, a wiadomo, że Java niszczy ludzkie mózgi gorzej niż najgorsza toksyna - ale mi osobiście taki kod czyta się znacznie lepiej.
tvNumber.text = "${position + 1}"
tvNumber.setBackgroundResource(getIconRes(isLevelPassed))
tvNumber.startAnimation(AnimationUtils.loadAnimation(tvNumber.context, R.anim.item_menu_show_animation))
tvNumber.setOnClickListener {
presenter.onLevelClicked(position, this)
}
Nie jest to błąd, ale zamiast powtarzać tvNumber, tvNumber, w Kotlinie bardziej elegancko jest użyć with lub apply.
Layouty
fragment_options mamy LinearLayout w LinearLayout w RelativeLayout. W aplikacji mobilnej łatwo wpaść w ten sposób w problemy wydajnościowe, a poza tym źle to o nas świadczy jako o programistach. Nie wiedział, jak używać layoutów, więc popakował wszystko w drabinę linearnych; tak jakby wszystkie elementy na stronie HTML pozycjonować sobie za pomocą tabelek ;) Tak że taką zagnieżdżoną strukturę warto by spłaszczyć. Przy odrobinie wysiłku powinno się z niej dać zrobić jeden RelativeLayout albo ConstraintLayout.txt czy img, bo TextView czy ImageView mają pewne unikalne właściwości. Ale stosowanie np. ll tylko dlatego, że coś jest widokiem typu LinearLayout, nie ma sensu. Z poziomu kodu nie wykorzystujesz żadnych szczególnych właściwości tego, że to jest akurat LinearLayout, a nie np. FrameLayout. Więc po co doprowadzać do przeciekania takich szczegółów implementacyjnych i zawracać czytelnikowi głowę zbędną informacją.Inne
Normalizer. Google it up. Nie trzeba robić tego na piechotę (AnswerChecker).BubblesProcessor pachnie niesławnymi "Managerami", bo "processor" to śliskie słowo-wytrych, skrót od: "ogólne takie takie" ;) Każdą odpowiedzialność można zmieścić w słowie "procesor"... w sumie cała aplikacja to jest pewnego rodzaju "procesor"....map { items ->
if (items.isEmpty())
{
throw IOException()
} else
{
items
}
}
map, bo nie zachodzi żadne mapowanie. Albo zrywasz cały strumień rzucając wyjątek, albo zwracasz to samo, co na wyjściu. Właściwym semantycznie operatorem byłby doOnNext. (To samo nadużycie występuje kilka linijek niżej). A po drugie, wyjątek nie ma zawiera żadnego zrozumiałego komunikatu. I dlaczego to jest właściwie akurat IOException? Ta sytuacja nie wygląda mi na błąd natury I/O. Możliwa alternatywa:
.doOnNext {
require(it.isNotEmpty()) {
"API returned no level packs!"
}
}
(ivPlay as ImageView).setImageResource(R.drawable.ic_play)
<include> (który nie wie, co jest pod spodem) na sam ImageView, a wtedy rzutowanie nie będzie potrzebne. Ta praktyka jest zła dlatego, że wystarczy, aby ktoś zmienił dany layout - np. opakowując ImageView w jakiś inny widok - aby spowodować crash, który wyjdzie na jaw dopiero w trakcie działania aplikacji.var passedLevels w LevelsPack. Nie prześledziłem całej ścieżki wykonania, ale starałbym się zrobić z tego val i w razie potrzeby inicjalizować nowy obiekt.BubblesSet - jest z pewnych przyczyn złą praktyką, i ja bym to z całego serca odradzał. (Oczywiście nie mówię o sytuacji, gdy rzeczywiście chcemy napisać własną, nową, uniwersalną kolekcję - ale w tym wypadku tak nie jest). Lepiej jest używać kompozycji. Dlaczego, możesz poczytać np. pod https://stackoverflow.com/questions/21692193/why-not-inherit-from-listt - gdzie zebrało się wiele świetnych odpowiedzi, w tym (zaakceptowana) Erica Lipperta. Pytanie dotyczy C#, ale nie ma to w tym wypadku znaczenia.Nazewnictwo i konwencje
...Contractor powinny mieć sufiks, czy też przyrostek, ...Contract, bo reprezentują pewien kontrakt. "Kontraktor" to jest pracownik kontraktowy. To trochę różnica, i to jest mylące. Nie rozumiem zresztą, jaki te interfejsy w ogóle mają sens, bo każdy z nich opakowuje tylko kontrakt widoku. Gdyby taki Contract zawierał np. interfejs prezentera oraz widoku, to by miał sens, a tak, to jest trochę "putting a hat on a hat" :)SharedPreferencesEditor nie jest dobrze nazwany, bo nazwa niepotrzebnie zdradza szczegóły implementacji. W rzeczywistości jest to tylko wrapper. który teoretycznie mógłby opakowywać np. zapis do SQL, i nie powinno to świata zewnętrznego obchodzić. Fakt, że np. LevelPacksPresenter stosuje SharedPreferencesEditor kiepsko wygląda, bo w MVP prezentery powinny być moim zdaniem izolowane od wszelkich zależności androidowych. Tu tak naprawdę jest, natomiast kiepska nazwa niepotrzebnie sugeruje, że jest gorzej, niż w rzeczywistości.data class Level(val levelID: Int. I potem w kodzie: level.levelID, masło maślane... Takie pole, czy cecha, powinno się nazywać po prostu id. Skoro jest zaimplementowane w klasie Level, to jest raczej oczywiste, że to musi być id obiektu Level, a nie jakiegoś innego ;)levelId, a nie levelID.Można by się czepiać dalej, ale opadłem już z sił. Ogólnie projekt nie jest zły, na twoim miejscu zapoznałbym się jeszcze bardziej z Kotlinem, zaczął pisać testy, unikał piętrowych layoutów oraz - dla rozwoju - popróbował MVVM.
Dzięki za tak szczegółową odpowiedź, zastosowałem się do wskazówek :). Sporo się z Twojego posta nauczyłem. Myślisz, że jest to wystarczający projekt, żeby dodać go do CV na stanowisko Junior Android Developer? Testy napiszę jak znajdę trochę wolnego czasu. Co do MVVM, to zaczynam się uczyć tej architektury (i Architecture Components), ale to był stary projekt napisany w MVP, który "odświeżyłem" przerabiając na Kotlina i zmieniając rzeczy, których się w międzyczasie nauczyłem.
Zakręcony Krawiec napisał(a):
Dzięki za tak szczegółową odpowiedź, zastosowałem się do wskazówek :). Sporo się z Twojego posta nauczyłem.
Spoko - cieszę się, że mogłem pomóc.
Myślisz, że jest to wystarczający projekt, żeby dodać go do CV na stanowisko Junior Android Developer?
Na juniora zdecydowanie, tym bardziej po poprawieniu głównych niedociągnięć.
Co do MVVM, to zaczynam się uczyć tej architektury (i Architecture Components), ale to był stary projekt napisany w MVP, który "odświeżyłem" przerabiając na Kotlina i zmieniając rzeczy, których się w międzyczasie nauczyłem.
Nie jest obowiązkowe, ale jest to tani sposób, by nabić sobie bonusowe punkty, bo MVVM - nie samo DataBinding, ale w połączeniu z Architecture Components - to jest rzecz względnie nowa, czyli sygnalizuje, że starasz się być na bieżąco.
A, byłbym zapomniał. Jeszcze niezbyt mi się podobało to rzutowanie na twardo hostującej Activity, żeby wstrzykiwać zależności do Fragmentów:
(activity as MainActivity).activityComponent?.plusMenuComponent(MenuModule())
menuComponent?.inject(this)
Czyli Fragment musi być w MainActivity, inaczej jest crash. Nie jest to eleganckie, bo wprowadza cyrkularną zależność: Activity wie o swoim Fragmencie, i vice versa. Fragment generalnie w ogóle nie powinien wiedzieć, w jakim jest Activity. Powinien raczej mieć swój własny moduł. Teoretycznie Fragment mógłby być reużywany w różnych Activity.
Tu parę artykułów, który pokazują, jak można użyć Daggera w czystszy sposób: