Cześć.
Mógłbym kogoś prosić o popatrzenie się w kod i jego ocenę?
Jest jeszcze parę rzeczy które chce dodać ale będę to sukcesywnie robił. Chciałbym wiedzieć czy na ta chwile kod jest ok i czy wrzucenie takiego projektu do CV będzie dobrze wyglądało?
Chciałbym zacząć szukać pierwszej pracy jako junior ewentualnie na staż iść byle już praktyczne doświadczenie łapać.
https://github.com/MatuidiDev/BooksWebstore
Wiem na pewno ze muszę napisać testy. Był to mój pierwszy tak duży projekt i ciężko mi było wymyślić testy przed napisaniem tego wszystkiego.

Hmm ogólnie jest ładnie i podział na pakiety, natomiast co mi się rzuciło w oczy tak na szybko:

• Dlaczego wybrałeś JSP? To dość stara technologia
• Czemu masz UserChangePasswordDTO, UserEditDTO, UserNewDTO? Nazwy klas powinny być rzeczownikami i reprezentować coś, jakiś właśnie rzeczownik. Niech ten User w sobie ma metodę changePassword(), ale nie rób do tego osobnego DTO.
• W interfejsach nie trzeba pisać public (przykłądowo w UserDAO) - jest to domyślne dla wszystkich metod.
• dlaczego SessionOrderService korzysta z requesta? request powinien być używany tylko w kontrolerach, w serwisie metody tutaj powinny od razu na sesji działać
• w BookServiceImpl w mtodach addBook i updateBook masz sporo logiki odnośnie tworzenia modelu - taka rzecz powinna być raczej w jakichś builderach
• w UserServiceImpl jak rzucasz exceptiony (i w sumie ogólnie gdzie rzucasz exceptiony) to niech to będą exceptiony bardziej wyspecjalizowane a nie ogólny Exception. No co najmniej RuntimeException żeby ktoś wyżej nie musiał tego obsługiwać

A tak to spoko ;)

Ja uwielbiam takie smaczki:

   @Override
    public void updateQuantity(Book book){
        bookDAO.save(book);
    }

Nie wiem co by się stało jakby tego save tam nie było.. :-)
(Zresztą to dotyczy prawie wszystkich ..save() ).

Z drugiej strony znam seniorów, którzy nie radzą sobie z napisaniem czegoś takiego... nawet mając tony bookstore spring mvc przykładów w necie...
Więc jak to sam zrobiłeś i to w kilka dni - to jesteś niezły i imo nadajesz się bardziej niż na juniora :-)
Za jakiś czas jak już będziesz miał prace poszukaj sobie tego obrazka:
https://tinyurl.com/zdkfg23
i spróbuj ogarnąć jego historię.

@Ziemiak mnie ciekawi tylko ile z tego kodu jest faktycznie "twoje" a ile przyszło z tutoriala :) Bo nad tym samym będzie sie zastanawiał rekruter później. Fajnie mieć jakiśtam projekt w CV ale generalnie ten kod to jest klasyczny generic CRUD. Gdybyś siadł i naklepał to od 0 to spoko, ale równie dobrze mógłbyś to zrobić kopiując kody z tutoriala.

Pinek napisał(a):

Hmm ogólnie jest ładnie i podział na pakiety, natomiast co mi się rzuciło w oczy tak na szybko:

• Dlaczego wybrałeś JSP? To dość stara technologia
• Czemu masz UserChangePasswordDTO, UserEditDTO, UserNewDTO? Nazwy klas powinny być rzeczownikami i reprezentować coś, jakiś właśnie rzeczownik. Niech ten User w sobie ma metodę changePassword(), ale nie rób do tego osobnego DTO.
• W interfejsach nie trzeba pisać public (przykłądowo w UserDAO) - jest to domyślne dla wszystkich metod.
• dlaczego SessionOrderService korzysta z requesta? request powinien być używany tylko w kontrolerach, w serwisie metody tutaj powinny od razu na sesji działać
• w BookServiceImpl w mtodach addBook i updateBook masz sporo logiki odnośnie tworzenia modelu - taka rzecz powinna być raczej w jakichś builderach
• w UserServiceImpl jak rzucasz exceptiony (i w sumie ogólnie gdzie rzucasz exceptiony) to niech to będą exceptiony bardziej wyspecjalizowane a nie ogólny Exception. No co najmniej RuntimeException żeby ktoś wyżej nie musiał tego obsługiwać

A tak to spoko ;)

@Pinek
Postaram się odnieść do wszystkiego
1.Myślałem nad Angularem2 ale najpierw musiałbym się go nauczyć a to znów zajęłoby trochę czasu a chce mieć już coś zrobione co bym mógł pokazać. A JSP znałem i tak wyszło.
2.To jest osobne DTO do każdego innego formularza.
3.Moje niedopatrzenie i rozpęd ;) W interfejsach od serwisów jest bez public a tu już z przyzwyczajenia napisałem. Mój błąd. Już poprawiony.
4.Ok. Poprawie. :)
5.Chodzi o wzorzec budowniczy?? W sumie dwa razy ten sam kod tam jest. Postaram się to jakoś przerobić.
6.Miałem metodę void i trochę z lenistwa tak zrobiłem żeby widzieć wyżej czy użytkownik z takim nickiem i emailem już istnieje. Zmienię to chyba na stringa.

jarekr000000 napisał(a):

Ja uwielbiam takie smaczki:

   @Override
    public void updateQuantity(Book book){
        bookDAO.save(book);
    }

Nie wiem co by się stało jakby tego save tam nie było.. :-)
(Zresztą to dotyczy prawie wszystkich ..save() ).

Z drugiej strony znam seniorów, którzy nie radzą sobie z napisaniem czegoś takiego... nawet mając tony bookstore spring mvc przykładów w necie...
Więc jak to sam zrobiłeś i to w kilka dni - to jesteś niezły i imo nadajesz się bardziej niż na juniora :-)
Za jakiś czas jak już będziesz miał prace poszukaj sobie tego obrazka:
https://tinyurl.com/zdkfg23
i spróbuj ogarnąć jego historię.

@jarekr000000
O kuffa :D Co to za magia :D Wywaliłem tego save i dalej działa :D w komentarzach widziałem jakiegoś linka, muszę o tym doczytać :)
I dziękuje ze miłe słowa ;)

i @Shalom Nie mówie ze tłukłem to od zera bo nie widzę sensu w wymyślaniu koła na nowo. Przejrzałem parę projektów tego typ, przeanalizowałem żeby znać zarys jak taka aplikacja powinna wyglądać i wtedy u siebie zacząłem pisałem. Starałem sie pisac wszystko sam jak tylko umiałem bez pomocy, a jak już było cos co musiałem szukać to zawsze to analizowalem, sam wkłepywałem bez kopiowania i ewentualnie poprawiałem jak mi sie cos głupie wydawało albo nie działało tak jak bym chciał. I w sumie nie robiłem tego na żadnym poradniku poza konfiguracja za pomocą Javy bo wcześniej zawsze robiłem na xml. W sumie to ta konfiguracja mi najwięcej zajęła czasu :D

Ziemiak napisał(a):

Pinek napisał(a):

• w BookServiceImpl w mtodach addBook i updateBook masz sporo logiki odnośnie tworzenia modelu - taka rzecz powinna być raczej w jakichś builderach

5.Chodzi o wzorzec budowniczy?? W sumie dwa razy ten sam kod tam jest. Postaram się to jakoś przerobić.

Tak chodzi o wzorzec. Tak naprawdę z tymi wzorcami jest tak że jak się rozumie architekturę to to się od razu stosuje, potem czasami się dowiadujesz że takie coś ma swoją nazwę jako jakiś wzorzec :D Z builderem chodzi o to żeby obiekt pewnej klasy był tworzony tylko w jednej konkretnej klasie, na podstawie propertiesów które mu dostarczysz. W sumie bardzo podobny do Factory

Kilka drobnych rzeczy:

BuyModel.java

• slaba nazwa
  isContains

• stringly typing
  updateQuantity(String[] amount)

BuyProduct.java

new BigDecimal(0) => BigDecimal.ZERO

BookDAO

literowka: findRandamBook

BuyOrderServiceImpl

.updateAmountInOrder
* Zwracanie błędu przez String:
* łączenie stringów
* brak internacjonalizacji

PictureServiceImpl

.addPicture, .updatePicture
* zahardcodowana ścieżka (kilka razy)
* wygaszanie wyjątku (zakładam że świadomie wybrane, chociaż wyjątki są np. w UserServiceImpl.addUser)
* logowanie na konsoli
* brak podania stosu w obsłudze wyjątku

PublisherServiceImpl

.deletePublisher
* wygaszanie wyjątku

UserServiceImpl

nazwa "username" zdegradowana - brak wydzielenia słów
zamień findByUsername na findByUserName
chyba że każde pole usera ma tak być prezentowane:
findByUserage
findByUserregistrationdate

Trzymanie wszystkich modeli w jednym pakiecie to kiepski pomysł. Dzielenie pionowe (Book*, User* itp) pozwala na lepsze odseparowanie odpowiedzialności i wykorzystanie dostępu pakietowego - klasy publiczne są tylko niewiele lepsze od pól publicznych.

Poprawiłem dość dużo zgodnie z waszymi sugestiami(przynajmniej tak myślę :D )
Najważniejsze to teraz BuyModel ma scope session i wyrzuciłem cale httpServletRequest
@vpiotr
Dzięki za tak dokładne pokazanie co do poprawy.

W userservice jest findByUsername bo mam pole username w User.

1. Dużo pustych linii - po co ci pusta linia na początku niektórych metod kontrolera? To samo w serwisie IMHO albo skasuj, albo extract method jeżeli warto
2. Skorzystaj z logerów zamiast logowania na standardowe wyjście
3. Przeleć kod sonarem, w nowej wersji instalacja jest bardzo prosta, a pewnie zaproponuje ci refaktor takich metod jak np. addProductToModel(BuyProduct product, Integer booksQuantity)
4. Może zastosuj lomboka? Zamiast generowania przez IDE metod toString i getterów/setterów/konstruktorów
5. metoda updateQuantity z BuyModel - usuwasz element w trakcie iteracji, ale nie przez Iterator.remove(), może wypluć ConcurrentModificationException
6. Może też czasami bardziej czytelne będzie zastosowanie streamów z Javy 8?
7. Kolega wyżej zasugerował ci zmianę new BigDecimal(0) na BigDecimal.ZERO, ale przynajmniej w konstruktorze BuyModel korzystasz z tego pierwszego
8. W BuyOrderDao zwróć generyczną listę tej metody find** to nie będziesz musiał później tego rzutować

gdybys udostepnil wsztskim pull requesta całego kodu ktory powstał do tej pory, łatwiej byłoby wytknac co do poprawy, jako początkujący fajnie ze łapiesz o co kaman w programowaniu ale burdel masz tam spory jeszcze.

BookServiceImpl

• przepisywanie manualne z dto do encji jest troche z d**y w tych czasach i zasmieca kod, albo wyekstrachuj converter albo uzyj zamiast tego jakiejs Oriki albo jDTO czy czego innego jeszcze. orika jest szybka i naprawde spoko

• jak dla mnie klasy typu service raczej nie powinny operowac na DTOsach a jedynie na encjach juz po konwersji czy tez odczycie, dzieki temu bedziesz mogl spokojnie uzyc servicu wewnatrz systemu bez potrzeby tworzenia DTOsow ktore tak naprawde sa uzywane tlyko na potrzeby mappnigu formularzy UI czy jakos tak

• skoro metoda updateQuantity jest pusta to po co zostawiłes ją ?

• jezeli jestes w book service, raczej nie musisz powtarzac za kazdym razem ze o ksiazke chodzi/jesli chodzi o ksiazke, jak np nazwa metody: findBooksForBooksBar -> findForBar or findForBooksBar

• return (List<Book>) bookDAO.findAll(); wydaje mi sei ze cos masz nie tak z dao, skoro musisz castowac wynik na list<Book>

• nie nadawaj niepotrzebnie nazw beanom springowym: @Service("BookService"), samo @Service wystarczy w przypadku jednej implementacji i @Autowired, najpierw resolution by type, zreszta on sam sobie nada podobną nazwe do twojej i uzyje w razie fallbacka do "resolution by name"

• jesli chodzi o kontrollery, no niestety uzalezniles sie od 'Model' i wprowadza to straszny zamet w kodzie, zupełnie niepotrzebny zamet.

• pamietaj tez ze jak przychodzi do kontrolera Long id, pozniej robisz recznie findById w kontrolerze, to mozesz to zamienic na uzycie konwertera i wtedy metoda kontrolera przyjmuje odarazu model który jest wyciagany po ID z uzyciem zarejestrowanego konwertera, konwerter piszesz raz i springMVC sam bedzie go uzywał wszedzie gdzie znajdzie przychodzace Long id z mozliwoscia przekonwertowania do konkretnego modelu

• jak masz w kontrolerze metody:

  • addNewUser, New i User w nazwie jest niepotrzbne bo skoro add to raczej bedzie zawsze new z definicji a ze jestes w UserService to wiadomo co dodajesz, chyba ze cos przeoczylem, nie musisz tez

  • @GetMapping("/delete/{username}") do delete nie uzywaj metody GET NIGDY, masz metode Delete w http przeciez, wtedy robisz cos ala @DeleteMapping(/{username}) i wtedy slowko delete w urlu jest niepotrzebne bo typ metody daje znac ze chodzi o delete, taka konwencja restowa
    

  • nie rozumiem dlaczego metoda getEditUserPage ma takiego urla: "change/password/{username}", to robisz get czy change ?

utworz pull requesta do jakiegos pustego brancha i daj dostep publiczny jakis, moze co nieco jeszcze pomozemy

hcubyc napisał(a):

4. Może zastosuj lomboka? Zamiast generowania przez IDE metod toString i getterów/setterów/konstruktorów

jezeli mam wpiac w aplikacje lomboka i uzaleznic wszysktich od tego liba, zdecydowanie wolałbym wpiac kotlina w projekt.
i na pewno lombok dla samego toStringa to przerost formy nad trescia

Co aktualnie zrobilem:

przepisywanie manualne z dto do encji jest troche z d**y w tych czasach i zasmieca kod, albo wyekstrachuj converter albo uzyj zamiast tego jakiejs Oriki albo jDTO czy czego innego jeszcze. orika jest szybka i naprawde spoko
jak dla mnie klasy typu service raczej nie powinny operowac na DTOsach a jedynie na encjach juz po konwersji czy tez odczycie, dzieki temu bedziesz mogl spokojnie uzyc servicu wewnatrz systemu bez potrzeby tworzenia DTOsow ktore tak naprawde sa uzywane tlyko na potrzeby mappnigu formularzy UI czy jakos tak

Dodalem konwertery
Chcialem wyrzuci DTO z serwisow i przesylac tam juz encje ale nie ogarniam troche tej oriki.
Jak wszystkie pola jakie sa w klasie User sa takie same w DTO to jest wszystko ok ale kiedy
w klasie DTO sa np tylko pola email, username, role to w obieckcie user dla pol ktorych nie ma w DTO wartosc sie zmienia z przyklodowo hasha hasla na Null
ustawienie w mapperze exclude("password") lub mapNulls(false) nie pomaga :/

skoro metoda updateQuantity jest pusta to po co zostawiłes ją ?

Dziala na "dirty checking" w hibernate ale wrocilem ja tak jak byla na poczatku. Jest to dla mnie duzo jasniejsze.

return (List<book>) bookDAO.findAll(); wydaje mi sei ze cos masz nie tak z dao, skoro musisz castowac wynik na list<book>

dao zwraca Iterable<book>. Dodalem w dao metode List<Book> findAll(); teraz jest ok bez rzutowania

nie nadawaj niepotrzebnie nazw beanom springowym: @Service("BookService"), samo @Service wystarczy w przypadku jednej implementacji i @Autowired, najpierw resolution by type, zreszta on sam sobie nada podobną nazwe do twojej i uzyje w razie fallbacka do "resolution by name"

Bede pamietal na przyszlosc

jesli chodzi o kontrollery, no niestety uzalezniles sie od 'Model' i wprowadza to straszny zamet w kodzie, zupełnie niepotrzebny zamet.

Czyli przejsc na ModelAndView czy jak?

pamietaj tez ze jak przychodzi do kontrolera Long id, pozniej robisz recznie findById w kontrolerze, to mozesz to zamienic na uzycie konwertera i wtedy metoda kontrolera przyjmuje odarazu model który jest wyciagany po ID z uzyciem zarejestrowanego konwertera, konwerter piszesz raz i springMVC sam bedzie go uzywał wszedzie gdzie znajdzie przychodzace Long id z mozliwoscia przekonwertowania do konkretnego modelu

Zrobione (w buyordercontroller zapomnialem w jednej metodzie przed commitem poprawic ale juz zrobione)

@GetMapping("/delete/{username}") do delete nie uzywaj metody GET NIGDY, masz metode Delete w http przeciez, wtedy robisz cos ala @DeleteMapping(/{username}) i wtedy slowko delete w urlu jest niepotrzebne bo typ metody daje znac ze chodzi o delete, taka konwencja restowa
nie rozumiem dlaczego metoda getEditUserPage ma takiego urla: "change/password/{username}", to robisz get czy change ?

porpawie to. Tylko musze wyslac zadanie Delete (Angularem to zrobie?) bo z tego co sie orientuje to "a href" tego nie zrobie

utworz pull requesta do jakiegos pustego brancha i daj dostep publiczny jakis, moze co nieco jeszcze pomozemy

Nwm czy dokladnie o to chodzilo. Zrobilem 2 branche, 4programmers jest wczesniejszy a master jest aktualny po commicie (Chyba powinno byc na odwrot nie? :D )

Dużo pustych linii - po co ci pusta linia na początku niektórych metod kontrolera? To samo w serwisie IMHO albo skasuj, albo extract method jeżeli warto

Jest to duzy problem? Staralem sie zeby kod byl dla mnie czytelny.

Skorzystaj z logerów zamiast logowania na standardowe wyjście

Zrobie jak bede mial chwile i przelece kod sonarem

Bede staral sie dalej porpawiac, Jednak nie ma to jak ktos bardziej doswiadczony powie co trzeba poprawic.
Aktualnie bede sie staral dodac testy oraz jakis kodzik w js bo nic w nim nie ma na stronie. I ciagle jeszcze poprawiac ten kod.

https://github.com/MatuidiDev/BooksWebstore/blob/master/src/main/webapp/WEB-INF/views/home.jsp

   <div id="wrapper">
            <div class="loginbar">
                <jsp:include page="loginbar.jsp" />
            </div>

            <div id="header">
                <jsp:include page="header.jsp" />
            </div>

            <div id="leftcolumn">
                <jsp:include page="leftmenu.jsp" />                
            </div>
            <div id="rightcolumn">

this is wrong on so many levels...

1. używanie id ma sens... dość rzadko. W większości przypadków o wiele lepszym rozwiązaniem są klasy CSS-owe (w sensie to klasy powinny być domyślną praktyką, przynajmniej jeśli chodzi o stylowanie - jeszcze w JS czasem ma sens używanie id do identyfikowania konkretnego elementu, ale szczerze mówiąc nie mam pojęcia jaką korzyść można odnieść w CSS korzystając z id, pomijając podbicie specyficzności w selektorach, ale i to można zrobić w inny sposób).
2. wrapper - co to za wrapper? Wszystko może mieć swój wrapper, samo wrapper jest dość słabą nazwą, niewiele to mówi (już nie wnikam w to, czy jest tu potrzebny w ogóle wrapper czy nie).
3. nazwy leftcolumn, rightcolumn też są słabe. Powodzenia przy robieniu widoku responsywnego na mobilki, gdzie lewa kolumna może stać się magicznie górnym rzędem. Lepiej byłoby nazwać jakoś semantycznie te kolumny.
4. alt="Brak zdjecia ksiazki" -- raczej powinno być "zdjęcie książki"

no i w stylach:
https://github.com/MatuidiDev/BooksWebstore/blob/master/src/main/webapp/resources/css/style.css

1. po co ten div? div.loginbar, div.headertext Całkiem zbyteczne i może jedynie utrudniać utrzymywanie takich klas (jeśli zamienisz w szablonie div na inny element, to ten selektor też będziesz musiał zmieniać za każdym razem).

2. .books tr td ul li a ten i podobne selektory za bardzo zależne od struktury HTML. Zmienisz w HTMLu cokolwiek, np. zamienisz tabelki na divy, albo zamiast listy <ul> zrobisz listę <ol> czy zamiast <a> zrobisz <button> i ten selektor będziesz musiał zmieniać.
   Tak samo tutaj, selektorki koszmarne w utrzymaniu i czytaniu: .login_table tr td input[type="text"], .login_table tr td input[type="password"], .login_table tr td input[type="email"

Przepisalem css. Mysle ze teraz jest bardziej czytelne.

Dobry pomysl nauczyc sie podstaw bootstrapa i angulara?

I potem zrobic restowy serwer (wtedy nie bylo by tych dziwnych konstrukcji /delete/{username}

Zaraz z tego co widziałem to Ty nie korzystasz z obiektów DTO tylko przesyłasz encje. Nie robisz transakcji na serwisach i w sumie nie wiem na co je stosujesz.