Skocz do zawartości
Sir Conceptz

Code review aplikacji do portfolio

    Rekomendowane odpowiedzi

    Chciałbym Was prosić o code review aplikacji na androida, która ma być w moim portfolio do szukania pracy jako junior dev. Aplikację wrzuciłem już na sklep Google Play, więc można śmiało pobrać i przetestować.

    https://github.com/sirconceptz/Pantry
    https://play.google.com/store/apps/details?id=com.hermanowicz.pantry

    Dzięki z góry za oceny, krytyczne również (w końcu nie ma lepszej oceny jak spojrzenie innej osoby na Twoją pracę).

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach
    Coderoid

    Pierwsze rzeczy które rzucają się w oczy:

    1. Brak jakiejkolwiek architektury (MVP, MVVM)

    2. Brak testów jednostokowych

    3. Brak dependency injection

    4. Brak innych libek, które pomagają w programowaniu (ButterKnife, retrolambda)

    5. Baza danych bez ORMa. Metody podatne na sql injection. Korzystaj z Room albo innego orma

    6. Statyczne metody w activity, dodatkowo publiczne. Potrzebujesz tego?

    7. MyPantryActivity - ifowisko, wszystko w jednej klasie, łacznie z adapterami

    8. Adaptery bez ViewHoldera.

    9. Nie odpinasz reklam. Zobacz tutaj https://developers.google.com/android/reference/com/google/android/gms/ads/AdView

    10. Ogólnie metody w aktywnościach są gigantyczne i robią wszystko. 

    • Piwko! 1

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach
    25 minut temu, Coderoid napisał:

    Pierwsze rzeczy które rzucają się w oczy:

    1. Brak jakiejkolwiek architektury (MVP, MVVM)

    2. Brak testów jednostokowych

    3. Brak dependency injection

    4. Brak innych libek, które pomagają w programowaniu (ButterKnife, retrolambda)

    5. Baza danych bez ORMa. Metody podatne na sql injection. Korzystaj z Room albo innego orma

    6. Statyczne metody w activity, dodatkowo publiczne. Potrzebujesz tego?

    7. MyPantryActivity - ifowisko, wszystko w jednej klasie, łacznie z adapterami

    8. Adaptery bez ViewHoldera.

    9. Nie odpinasz reklam. Zobacz tutaj https://developers.google.com/android/reference/com/google/android/gms/ads/AdView

    10. Ogólnie metody w aktywnościach są gigantyczne i robią wszystko. 

    Dzięki bardzo za rzeczową odpowiedź. Postaram się wszystko poprawić. Raz jeszcze dzięki.

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach

    @Wafii

    1. Nazwa pakietu bez "_". Nazwa aplikacji też konsekwentnie. Albo ToDoList, albo To-Do-List.

    2. Ikony (ic_app-web.png, app_icon-web.png) przenieś do  odpowiednich katalogach.

    3. Dodaj testy.

    4. Nie zostawiaj zakomentowanego kodu.

    5. MainActivity context = this; <- wszedzie gdzie potrzeba możesz this użyć wprost ;)

    6. TaskClass - po prostu może to być Task.

    7. NotificationHelper helperNotification dlaczego nie notificationHelper?

    8. Wszystkie stringi typu: "Delete all selected tasks", "Are you sure?" do strings.xml

    9. listTasks.remove(selectedTask.get(i));  bez pętli -> listTasks.removeAll(selectedTask);

    10. metoda sendNotification() nie jest używana

    11. Dodaj bazę danych i nie zapisuj w SharedPreferences. Korzystaj z ORM!

    12. Musiałem podbić gradla żeby zbudować projekt u siebie (dziwne... korzystam z AS 3.3)

    13. Korzystaj z podpowiedzi AS:
    image.png.e9b16155646a935fef0ccb57de86e690.png

    14. Poznaj linta :) Uruchom sobie w task gradla: gradlew lint. Dostaniesz raport z problemami które ma Twoja aplikacja: lint-result.html

    15. Podoba mi się Twój RecyclerView ;)

    • Piwko! 1

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach
    Wafii

    @Coders Lab

    Godzinę temu, Coders Lab napisał:

     

    Dziękuję za szczegółową odpowiedź. Mam jeszcze jedno pytanie. Wszystkie kolor definiować w colors.xml? Jeśli tak to czy można zrobić jakby sekcję kolorów dla całej aplikacji? Nie pisać, że np.

    <!-- login_activity-->
    <color name="background_login_activity">#000</color>

    <color name="icon_color">#6B6B6B</color>

     

    <!-- register_activity-->
    <color name="background_register_activity">#FFF</color>

    <color name="icon_color">#6B6B6B</color>

     

    tylko:

    <!--application_colors-->

    <color name="icon_color">#6B6B6B</color>

    <!--login_activity-->

    <color name="background_login_activity">#000</color>

    <!-- register_activity-->

    <color name="background_register_activity">#FFF</color>

     

    Mam nadzieje, że przejrzyście to przedstawiłem(te nazwy powinny być chaba krótsze :))

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach

    @Wafii Kolory powinny być spójne dla całej aplikacji. Jeżeli tego samego koloru będziesz używał w kilku miejscach, lub do tych samych elementów (w Twoim przypadku np. ikon) nazwa powinna na to wskazywać. Podział na sekcje, który zaproponowałeś, według mnie nie jest zły. Pamiętaj żeby znajdowały się tam kolory używane tylko w tych sekcjach. Jeśli chodzi o nazwy to są w porządku - jasno wskazują gdzie dany kolor jest używany ;) 

    Udostępnij tę odpowiedź


    Odnośnik do odpowiedzi
    Udostępnij na innych stronach

    Jeśli chcesz dodać odpowiedź, zaloguj się lub zarejestruj nowe konto

    Jedynie zarejestrowani użytkownicy mogą komentować zawartość tej strony.

    Zarejestruj nowe konto

    Załóż nowe konto. To bardzo proste!

    Zarejestruj się

    Zaloguj się

    Posiadasz już konto? Zaloguj się poniżej.

    Zaloguj się

    • Ostatnio przeglądający   0 użytkowników

      Brak zarejestrowanych użytkowników przeglądających tę stronę.

    x