Wątek przeniesiony 2020-08-27 12:13 z Java przez Shalom.

Code review web-aplikacji

0

Witam, poszukuję pracy jako junior developer i podsyłam do oceny moje aplikacje z CV.

Będę wdzięczny za porady i krytykę.

Mapa przedstawiająca liczbę zarażonych 2019-nCoV.
Server: https://covid-2019-map.herokuapp.com
Code: https://github.com/DarX1221/covid-map

Aplikacja webowa służąca do hostingu zdjęć.
Wspiera rejestrację, logowanie, konto administratora i galerię użytkownika.
Server: https://image-web-app-test.herokuapp.com
Code: https://github.com/DarX1221/image-web-app-test
konto admin: admin1 pass:admin1

Mam kilka konkretnych pytań:

  1. Czy pozostawienie pakietu com.example to duży błąd, nigdy na to nie zwracałem uwagii

  2. Na tutorialach zawsze pakietyzacja przebiega względem tego czym dana klasa się zajmuje
    pakiety: Contoller, Model, Repo, Service

ostatnio wyczytałem że powinno się pakietyzować według modeli
pakiety: User, Book .... i w tych pakietach powinny się znajdować klasy: Controler, Service ...

  1. W mojej drugiej aplikacji image-web-app, podwójne klniknięcie, rzuca wyjątkiem, czy taka obsługa tego wyjątku jest poprawna
public boolean removeImageById(Long id) {
    Authentication auth = SecurityContextHolder.getContext().getAuthentication();
    String username = auth.getName();
    // if logged username is different than username in AppImage(Long id) to delete, you can delete image only if you are admin
    if (!imageRepository.findAppImageById(id).getAppUser().getUsername().equals(username)) {
        if (!userService.checkTahtLoggedUserIsAdmin()) {
            return false;
        }
    }
    // double click "delete image", try to remove image two times
    try {
        String cloudinaryId = imageRepository.findAppImageById(id).getCloudinaryId();
        imageUploaderService.deleteImage(cloudinaryId);
        imageRepository.removeAppImageById(id);
        return true;
    } catch (InternalAuthenticationServiceException doubleClickDeleteEsception) {
        throw new InternalAuthenticationServiceException(
                "This exception was throw by double click on delete image button");
    } catch (Exception ex) {
        ex.printStackTrace();
    }
    return false;
}
2

Co do samego kodu to się nie wypowiem, bo Java to nie moja bajka (aczkolwiek uważam, że wywalanie zakomentowanych linii przed publikacją jest dobrym zwyczajem, niezależnie od języka), ale oceniając to, co pokazałeś z perspektywy użytkownika to jest bardzo słabe.

  • Skoro chcesz to pokazywać jako portfolio, to przyłóż trochę uwagi do tego, jak to wygląda. Bo na razie masz po prostu wrzucone parę elementów, ale wygląda to okropnie.
  • Hosting grafik powinien oferować (poza jakimkolwiek wyglądem) chociażby możliwość pobrania URL danego zdjęcia, żeby je gdzieś wsadzić/podlinkować. Chyba, że nie ma to być jedynie na użytek wewnętrzny - ale wtedy pytanie, po co takie coś robić? Zwłaszcza, że nie ma żadnego tagowania, wyszukiwania, więc przy kilkudziesięciu elementach to po prostu będzie niezdatne do użytkowania
  • Nie powinieneś podczas przeglądania już posiadanych zdjęć w galerii wrzucać całych obrazków, ale miniaturki z możliwością powiększenia
  • Mapa z niewiadomego powodu zajmuje pół ekranu, a dolna część (ok. 50%) to biała przestrzeń z informacją o dacie aktualizacji danych. Nie da się przesunąć linii podziału, więc pół ekranu się marnuje. Myślałem, że może ta przestrzeń jest wykorzystywana po kliknięciu, żeby pokazać statystyki, ale nie - one się pokazują w dymku na mapie. Jest to albo jakiś błąd, albo totalnie nieprzemyślany layout strony
  • Formularz logowania masz ładnie ostylowany, ale rejestracja to brzydki, goły HTML. FUJ!

screenshot-20200827130020.png


screenshot-20200827125934.png


screenshot-20200827130156.png


screenshot-20200827130233.png

2
DarK1221 napisał(a):
  1. Na tutorialach zawsze pakietyzacja przebiega względem tego czym dana klasa się zajmuje
    pakiety: Contoller, Model, Repo, Service

ostatnio wyczytałem że powinno się pakietyzować według modeli
pakiety: User, Book .... i w tych pakietach powinny się znajdować klasy: Controler, Service ...

Zgadza się, podejście tzw package by FEATURE jest łatwiejsze do znajdywania tego co chcemy w projekcie.
Tylko za daleko wychodzisz. Przecież masz tylko 2 klasy (pomijam startowanie apki): https://github.com/DarX1221/covid-map/tree/master/src/main/java/com/example/demo
Co tam chcesz dzielić?

Testy testami, ale tu nie ma praktycznie czego testować bo ilość kodu jest znikoma.
Dodatkowo zaprzestałbym się bawić w Model. Lepiej zwracać REST Api a potem obsługiwać to na niezależnym froncie.

EDIT: dobra, mój błąd, przeoczyłem drugie repo. Ale tam ilość kodu też nie powala. Potem może naskrobię coś więcej.

0

Dzięki wszystkim za odpowiedź, ogarnę to co zostało tu wymienione.

@cerrato
Tak, rzeczywiście wygląda to dosyć obskurnie, na swoją obronę dodam że dotychczas uczyłem się backend'u, i chęć zrobienia portfolio wymusiła zrobienie czegoś w czym się na razie nie odnajduje.
(PS: u mnie na lapku ta mapa zajmuje prawie cały ekran, muszę ogarnąc aby się skalowała w pionie)

@Charles_Ray
Szczerze mówiąc to nie wiem nawet jakie testy zrobić, ale pomyśle nad tym.
Co do hasełek to pomyślałem o tym, stworzyłem drugą aplikację (testową) i umieściłem ją tutaj i na heroku, aplikacja w moim portfolio jest tylko na heroku i sam nie mam wglądu do jej kodu. Czyli ta pka jest na odzielnym serwerze, korzysta z innej DB i chmury. Choć pewnie lepszym rozwiązaniem byłoby zaszyfrowanie ich w plikach aplikacji.

@kixe52
Dokładnie w pierwszym repo nawet nie zawracałem sobie głowy pakietami.

Dodatkowo zaprzestałbym się bawić w Model. Lepiej zwracać REST Api a potem obsługiwać to na niezależnym froncie.

Nie bardzo rozumiem, z mojej perpektywy potrzebuję modelu do stworzenia encji (zmapowania modelu do bazy danych), ale pomijając to.
W jakim froncie da się to ogarnąć, w tym momencie uczę się thymeleaf'a, ale on jest raczej czymś co spaja html z javą (springiem).

2
DarK1221 napisał(a):

Nie bardzo rozumiem, z mojej perpektywy potrzebuję modelu do stworzenia encji (zmapowania modelu do bazy danych), ale pomijając to.
W jakim froncie da się to ogarnąć, w tym momencie uczę się thymeleaf'a, ale on jest raczej czymś co spaja html z javą (springiem).

Nie chodziło mi o encję bazodanową a o to: https://github.com/DarX1221/image-web-app-test/blob/master/src/main/java/com/example/demo/controller/GalleryAllImageControler.java

import org.springframework.ui.Model;

Na twoim miejscu zaprzestałbym naukę thymeleafa, to nie te lata. Naucz się robić porządnie REST Api. Do testowania nie potrzebujesz koniecznie frontu, na start wystarczy postman.
Jak już to ogarniesz to bierz się za front.
Polecam angulara (nowsze wersje na typescriptcie). Jeżeli ogarniasz javę to TS też szybko załapiesz. Dużym ułatwieniem są wspomagacze typu 'angular materials' itp dzięki którym nie musisz walczyć z cssami. Jak wpiszesz w google java spring + angular to myślę, że znajdziesz wiele tutoriali od których możesz zacząć przygodę. Chociażby żeby zobaczyć jak to połączyć. A samego angulara dobrze jest się uczyć z oficjalnej dokumentacji + dużo googlować.

3

Mapa. Pierwsza klasa z brzegu: CovidDataService.

  1. Statyczne metody bardzo utrudniające stworzenie testów jednostkowych
  2. Brak interfejsu do implementacji -> brak odseparowania implementacji od interfejsu -> brak IoC
  3. Zaszycie url ze źrodłem danych w kodzie (powinien być w konfiguracji)
  4. Rozdzielenie ściśle ze sobą powiązanych danych (longitude/latitude/description), powinny siedzieć w jednym modelu (którego brak) i powinieneś mieć jedną tablicę tego modelu. Odseparowanie powiązanych danych oznacza zgodę na ich niespójność (np. brakujące/nadmiarowe dane w jednej tablicy).
    5 .W nazwach tabel trzymających powyższe dane wyeksponowane są najmniej istotne rzeczy: pointList. Najbardziej istotna, czyli to co element faktycznie trzyma, jest skrótem. Dodam, że żadna z tych list nie jest listą punktów (prefiks "point").
  5. getCovidData robi wszystko, ściąga dane (tworząc wszystkie "niskopoziomowe" obiekty do tego niezbędne), przetwarza, ładuje do wynikowych list. Polecam świętego Graala wszystkich juniorów i midów - książkę "Clean code". Rozepnij to na kawałki. Każda metoda wykonuje operacje tylko na jednym poziomie abstrakcji. Każda metoda zajmuje się tylko jedną czynnością. Każda klasa zajmuje się tylko jednym rodzajem rzeczy. SRP i SOLID. Zrobi Ci się z tego kilkanaście metod mających po 2-4 linijki oraz kilka klas.
  6. Losowe wcięcia (np. raz spacja po if, raz jej brak)
  7. Metody mają nazwy zawierające czasownik (bo metoda coś robi), podczas gdy pola/właściwości/zmienne lokalne nie mają czasownika, bo nic nie robią, tylko przechowują/opakowują dane. CSVParser parse powinno się nazywać parsedData lub parsedCovidData lub coś w tym stylu
  8. Co to jest casses? Dlaczego zawiera datę? Dlaczego data (dateStr) jest generowana w - potencjalnie - każdym przebiegu pętli?
  9. getPointListLat wywołuje zaciągnięcie danych, pozostałe "gettery" tego nie robią, dlaczego? W żaden sposób nie informujesz, że najpierw trzeba zaciągnąć listę szerokości geograficznych. Ten brzydki, bo głęboko schowany, błąd jest ściśle powiązany z nieprawidłową separacją powiązanych ze sobą danych (vide 4).

Dalej mi się nie chce.

Lektura: pojęcia DRY, KISS, SOLID + zakup książki "Clean code".

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