Ocena kodu Java

Witam,
Proszę o ocenę kodu.
https://github.com/giecmarcin/ProjectManager

1. Invitation.setIsAccept - osobiście preferuję (oraz częściej widuję się) z nazewnictwem w formie przeszłej: accepted, zatem po poprawkach: setIsAccepted, a najlepiej po prostu setAccepted (podobnie getter - isAccepted).
2. Invitation.setEmailOfUser bleh - setUserMail/setUserEmail (skłaniałbym się raczej pisania mail, a nie email).
3. Invitation.setIdProject bleh x2 - setProjectId/getProjectId.
4. Podobnie jak cała ta reszta w stylu getDateStart - getStartDate etc. Oczywiście z jednej strony to zależy kto jaki styl preferuje, lecz ja częściej spotykam się z kolokacjami przymiotnik+rzeczownik, a nie odwrotnie.
5. Masz każdy plik okomentowany Created by Marcin on 2015-09-25., lecz w żadnym tak naprawdę nie ma sensownego komentarza, jaka klasa jest zawarta w tym pliku, do czego służy, brakuje opisów metod etc.
6. Pole Task.progress jest doublem - uważałbym z tym, lepiej już zapisywać jako int (czy tam uint8 czy co tam ma Java w zakresie 0-100), aby nie stracić na dokładności (nie wiem jak tam dokładnie to wykorzystujesz, więc ten punkt może mieć sens, albo też zupełnie odbiega od tematu :P).
7. Masz User.name oraz User.lastname, co skutkuje drobnym zamieszaniem, co tak naprawdę określa pole name. Albo zrób firstname/lastname, albo zostaw samo name.
8. [ProjectDaoImpl.java:68] naprawdę uważasz komentarz //now you have one array of Object for each row za niezbędny? Czy kod bez niego jest mniej czytelny?
9. [UserDaoImpl.java] nieładnie jest uciszać wyjątki - dodaj tam przynajmniej jakieś logowanie tego (w sensie: zapisywanie do pliku logu, że wystąpił taki a taki wyjątek przy takim a takim parametrze).
   To tak pobieżnie - moje wskazówki są dosyć ogólne odnośnie stylistyki; nie zwracałem większej uwagi na przykład na potencjalne dziwne wykorzystania frameworka, ponieważ nie znam się na Javie czy też Springu, lecz jakiś zarys drobnych wtfów powinien Ci się naświetlić.

Rzuciłem tylko okiem ale:

1. Praktycznie pusta warstwa logiki, za to w DAO widze jakieś cuda na kiju...
2. Niespójne i generalnie niskie zakładania transakcji. Transakcje zakłada sie na serwisach a nie na DAO
3. Springowe adnotacje podczas gdy od dawna mamy już @Named czy @Inject
4. Łapanie wyjątków i zwracanie nulli o_O raz że od dawna jest Optional, a dwa że dzięki temu nie wiesz w ogóle co to za błąd wystąpił ;]
5. Niby masz ORMa a walisz tam jakieś native query.
6. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/service/impl/CommonServiceImpl.java nie ma to jak ręczna internacjonalizacja :D :D
7. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/service/impl/InitDbServiceImpl.java to się da zrobić za pomocą skryptu startowego dla bazy danych...
8. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/repository/impl/ProjectDaoImpl.java takich rzeczy nie robi się w DAO. Dao jest od pobierania danych z bazy
9. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/controller/LoginController.java przyjmowanie tych obiektów jako parametrów bez potrzeby to raczej zła praktyka
10. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/controller/OtherProjectsController.java Powinieneś zwracać ModelAndView a nie bawić się w przyjmowanie modelu jako argumentu
11. https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/controller/ProjectController.java te twoje ifologie tutaj powinny być w warstwie serwisów bo to wszystko co wepchnąłeś do processAddNewProjectForm to jest LOGIKA aplikacji. Analogicznie w innych kontrolerach. Kontroler ma zawierać ZERO logiki. Kontroler ma:

• odebrać parametry
• zawołać odpowiednie serwisy
• spakować wyniki
• posłać dane do widoku
• ewentualnie wyświetlić błędy

    @Override
    public List<User> getUsersFromProject() {
        TypedQuery<User> query = em.createQuery(
                "select p from Project p where p.emailOfCreator = :email", User.class);
        query.setParameter("email", "Test");
        List<User> result = query.getResultList();
        if (result.isEmpty() || result == null) {
            return null;
        } else {
            return result;
        }
    }

Nie lepiej tak jak poniżej? Przynajmniej nie zwrócisz nulla zamiast pustej listy

    public List<User> getUsersFromProject() {
        TypedQuery<User> query = em.createQuery(
                "select p from Project p where p.emailOfCreator = :email", User.class);
        query.setParameter("email", "Test");
        return query.getResultList();

z https://github.com/giecmarcin/ProjectManager/blob/master/src/main/java/com/springapp/mvc/repository/impl/ProjectDaoImpl.java od linii 65 tego whila mógłbyś wrzucić do innej metody o nazwie np. initializeProject(), prepareProject(), od bidy mógłbyś w Project stworzyć konstruktor o takich parametrach

Trochę formatowanie mógłbyś poprawić, bo zdarzają się np. podwójne puste linie
Konstrukcje if-else zmieniłbym na ternary zwłaszcza jak coś zwracasz, ale to wedle uznania imho

Dzięki wielkie, postaram się poprawić.

Jeszcze mój szybki rzut oka:

1. Osobiście preferuję inny styl nazewnictwa stron, zamiast www.example.com/otherProjects.jsp stosuję www.example.com/other-projects.jsp.
2. Jeśli pojawia się nazwa klasy w stylu InvitationServiceImpl to już moim zdaniem jest to tzw. "code smell". Czy faktycznie potrzebujesz interfejsu oraz jednej implementacji tego interfejsu?
3. Zamiast:

Set<Task> tasksForCurrentUser = new HashSet<Task>();

Piszemy:

Set<Task> tasksForCurrentUser = new HashSet<>();

4. Jak już wspomniał Shalom - internacjonalizacja napisów do przebudowy :)

@Marszal

Czy faktycznie potrzebujesz interfejsu oraz jednej implementacji tego interfejsu?

Widać że słabo znasz Javę EE/Springa a przynajmniej takie pojęcie jak dynamic proxy i AOP. Jeśli ktoś nie chce kombinować z cglibem i class-proxy (które też nie zawsze może być użyte) to standardowym podejściem jest stosowanie interfejsów i wstrzykiwanie przez interfejsy. Dzięki temu nie ma problemów z robieniem proxy bo framework może sobie bez problemów implementować interfejs.
I od razu uprzedzam, ze to nie są jakieś wydumane hardkorowe rzeczy -> ot każde użycie @Transactional wymaga generacji dynamic proxy.

Shalom napisał(a):

3. Springowe adnotacje podczas gdy od dawna mamy już @Named czy @Inject

Możesz rozwinąć?

A co tu rozwijać? Od dawna do standardu javy weszło CDI i tym samym standardowe javowe adnotacje do wstrzykiwania zależności i definiowania lokalnych beanów a Springa można uzywać jako dostawcy CDI w tym kontekście. To tak jak uzywanie Hibernate przez JPA albo używanie Hibernate samodzielnie.

Nigdy nie spotkałem się z tym żeby któs krytykował korzystanie ze Springowych adnotacji w springowym projekcie zamiast tych + literatura a dokładniej Spring in Action sugeruje korzystanie ze Springowych, gdyż np. Named zdecydowanie malo mowi ze wzgledu na swa kiepska nazwe

Sorry za OT. Jak przeglądam kod w RoR czy PHP czy innym języku/frameworku to zwykle coś rozumiem, a tu ni cholery. Czarna magia. Dlaczego ta Java jest tak zawiła i nieprzystępna? Kto to wymyślił i po co aż tak utrudnił życie programistom? :) Przyznacie, że mam sporo racji....

@Wielki Samiec zauważ że jak się nauczysz korzystać z CDI to będziesz mógł pracować z dowolnym dostawcą CDI ;)

Po prostu trzeba się w to wczuć i wiedzieć, co gdzie przychodzi. Dla programisty Javy to układ Zend Frameworka będzie idiotyczny :P - Patryk27 44 minuty temu

Jasne, ale przeglądam przykłady ma github i ich rezultaty (działające demo) i myślę, że w wielu językach można by zrobić podobne apki zapisując znacznie mniej linii kodu. Podobno w Javie jest tylko jeden sposób (poprawny) na napisanie aplikacji - to na pewno jest zaleta, ale z drugiej strony API Javy jest przytłaczające.