Prośba Github/Code review.

Odpowiedz Nowy wątek
2019-02-20 20:25
0

Cześć,

proszę o ocenę mojego Github-a oraz mojego głównego projektu.
Jest to pierwszy, większy projekt więc dużo do krytyki pewnie będzie :)

Aplikacja to sklep internetowy, w którym jest podział na role, typowe CRUD itp.
Użyte technologie to m.in Spring Boot, Spring Security, Hibernate, MySql, Thymeleaf.
Szczegółowo jest opisane na githubie https://github.com/kosonm/OnlineStore-Deployed-
Poklikać można na https://eshop-kosonm.herokuapp.com (może zająć ~30s bo odpalone na darmowym dyno).

Też proszę o ocenę 'wyglądu' mojego Github-a, czy dobrze są opisane projekty itp.
https://github.com/kosonm

I pytanie czy taki sklep internetowy zainteresuje potencjalnego pracodawce? Dużo osób robi podobne projekty w trakcie nauki.

Dzięki!

edytowany 1x, ostatnio: samolocyk, 2019-02-20 20:26
jeśli masz maila studenckiego mozesz ogarnac sobie https://education.github.com/students i postawic jedno dyno płatne za darmo - danek 2019-02-25 08:43

Pozostało 580 znaków

2019-02-20 21:14
0

O, coś większego :) ogólnie całkiem spoko jak na pierwszy większy projekt, tak na szybko (nie jestem w pracy żeby mieć czas więcej napisać :P )

  • nazwy paczek z małej litery (konwencje)
  • nie używaj Autowired tylko ctor injection
  • podziel kontrolery na mniejsze bo jeden z nich jest ogromny
  • w orderList możesz przecież normalnie databindować typ int bez tego dziwnego try-catch
  • prawdopodobnie użycie tych magicznych JpaRepository z JPA wyeliminowałyby większość ręcznych SQL-i w DAO (choć tu nie jestem pewien), to klejenie SQL w stringach też średnio
  • jeśli chcesz mieć typowy anemiczny model domeny, to użyj Lomboka, który wyeliminuje te wszystkie gettery i settery z encji
  • nie wiem czy VSCode pokazuje nieużywane metody, ale IJ widzi ich całkiem sporo - można je wywalić

Bardzo duży plus za działające demo! Zwróć też uwagę na walidacje na froncie, chyba wpisałem jakieś znaki w 'code' i dostałem brzydki exception na twarz :)

edytowany 1x, ostatnio: kelog, 2019-02-20 21:15
nie używaj Autowired tylko ctor injection - rozwiniesz? Przerabiam kurs, który traktuje o Autowired, o Inject cisza - Burdzi0 2019-02-20 21:44
Tu chodzi zapewne - o constructor injection - czyli nieco mniejszy poziom raka. Autowired czy Inject to jeden pies. Nawet jak się je pominie (bo przy konstruktorze można). - jarekr000000 2019-02-20 21:52
Okej, poza "Autowired czy Inject to jeden pies" wszystko wiedziałem, czyli jakiś progres jest - Burdzi0 2019-02-20 22:25

Pozostało 580 znaków

2019-02-20 21:46
0

Ode mnie w sumie jedno - 15 commitów dotyczy tylko i wyłącznie Readme. Konsekwentne używanie gita w projekcie może być uważane za plus ;)


Pozostało 580 znaków

2019-02-20 21:59
3

Zrobiłem review na live - będziesz wiedział.


Bardzo lubie Singletony, dlatego robię po kilka instancji każdego.
zabieram się do czytania o xss (y) - samolocyk 2019-02-21 02:34

Pozostało 580 znaków

2019-02-20 22:23
3
  1. Dzielenie na warstwy wyszło już z użycia, kiedyś była to powszechna praktyka, teraz robi się pakiety per domena, czyli w twoim przypadku pakiet cart, order, customer zamiast model, service itp. Korzyści z tego, masz porządek, wszystko dotyczące klienta w jednym pakiecie, jak np. to wszystko dotyczące klienta się rozrośnie, możesz łatwo zrobić refaktor i nowy websevice wydzielić.

  2. nazwy pakietów z małej litery (to tylko taka konwencja programistów javy)

  3. Używaj dobrodziejstw javy 8+, np. stream().filter
    Przykład zamiast:

    private CartLineInfo findLineByCode(String code) {
        for (CartLineInfo line : this.cartLines) {
            if (line.getProductInfo().getCode().equals(code)) {
                return line;
            }
        }
        return null;
    }

    możesz napisać:

    private Optional<CartLineInfo> findLineByCode(String code) {
        return cartLines.stream()
                .filter(line -> line.getProductInfo().getCode().equals(code))
                .findFirst();
    }

    Nawiasem mówiąc optional też jest spoko zamiast nulla.
    Jeszcze to get get jest słabe, to też można poprawić np. wprowadzając w klasie CartLineInfo metodę getProductCode i wtedy to wygląda tak:

    private Optional<CartLineInfo> findLineByCode(String code) {
        return cartLines.stream()
                .filter(line -> line.getProductCode().equals(code))
                .findFirst();
    }

    coraz lepiej co nie :)

  4. Metody addProduct , updateProduict spodziewałabym się w osobnej klasie. Mieszasz dane z logiką biznesową.

  5. Zamiast:

    if (code != null && code.length() > 0)

    możesz zapisać (to jest to samo, ale metoda już nazwana i wygląda czytelniej):

    if (StringUtils.isNotEmpty(code))
  6. Masz (to jest ok):

    public ProductForm() {
        this.newProduct= true;
    }

    ale potem bez sensu ustawiasz newProduct, bo już jest true

    productForm = new ProductForm();
    productForm.setNewProduct(true);
  7. Zamiast:

    public Product findProduct(String code) {
        try {
            String sql = "Select e from " + Product.class.getName() + " e Where e.code =:code ";
            Session session = this.sessionFactory.getCurrentSession();
            Query<Product> query = session.createQuery(sql, Product.class);
            query.setParameter("code", code);
            return (Product) query.getSingleResult();
        } catch (NoResultException e) {
            return null;
        }
    }

    możesz użyć klasy repozytorium z arsenału gotowców springa:

    public interface ProductRepository extends CrudRepository<Product, Long> {
    List<Product> findByCode(String code);
    }

    To samo, a mniej kodu, większa czytelność.

  8. to ciągłe sprawdzanie != null wygląda słabo

  9. poczytaj o testach jednostkowych i Junit

  10. wstrzykiwanie przez konstruktor

  11. EntityManager lepsze od SessionFactory:
    https://stackoverflow.com/que[...]ctory-vs-entitymanagerfactory

itd. ale na dzisiaj starczy :)

Zamiast .findFirst(); powinno byc .findAny(); o ile nie zalezy Ci na kolejności zwraca wtedy dowolny pasujący element a nie przelatuje całego strema jak dobrze pamietam - Ziemiak 2019-02-22 19:05

Pozostało 580 znaków

2019-02-21 00:13
0

Dzięki wielkie wszystkim, szczególnie @szarotka i @kelog :)
Patrząc na to co napisaliście trochę mam do zrobienia. Poprawiłem już te najprostsze rzeczy (Lombok,nazwy pakietów,podział kontrolerów). Jutro pod wieczór zaktualizuję na githubie jak więcej zrobię i dam tutaj znać.
Na razie muszę poczytać o wstrzykiwaniu przez konstruktor i EntityManager vs, SessionFactory (dzięki za link @szarotka) :P

edytowany 1x, ostatnio: samolocyk, 2019-02-21 00:14

Pozostało 580 znaków

2019-02-22 18:34
0

Szybki update, zastosowałem się już do sporej części rad.

  1. Wstrzykiwanie przez konstruktor.
  2. Pakiety per domena (chyba dobrze zrozumiałem) i z małej litery.
  3. Ochrona przed XSS (przynajmniej tak mi się wydaje) - użyłem 'Spring HtmlUtils' w validatorach.
  4. Lombok, redukcja kodu typu gettery/settery.
  5. Zamiana SessionFactory na EntityManager
  6. Podzielenie kontrolerów na mniejsze.

Następnie: pętle na stream-y, zająć się tymi 'nullami' i pozamieniać moje funkcje gdzie "kleję Stringi" na gotowe repozytoria.

Pozostało 580 znaków

2019-02-22 18:45
1

Ochrona XSS jest zrobiona niepoprawnie.

  • Validacja na wejściu jest opcjonalna/przydatna.
  • Encoding na wyjściu jest wymagany!

Ponieważ/bo...
może kiedyś, ktoś do programu ktoś dorobi jakieś inne importy /serwisy - nie poprzez formularz web. I np. zapomni dodać tej walidacji (w jego przypadku nie bedzie miała sensu).
Takie błedy zdarzają się w poważnych serwisach - na stronie www jest walidacja... ale w aplikacji mobilnej na android już nie i hulaj dusza.

na pewno na owasp jest cheatsheet dla XSS, zastosuj sie.


Bardzo lubie Singletony, dlatego robię po kilka instancji każdego.
edytowany 4x, ostatnio: jarekr000000, 2019-02-23 00:05

Pozostało 580 znaków

2019-02-27 11:05
V-2
0

Przejrzałem na wyrywki, ale już na pierwszy rzut oka, estetyka nie robi najlepszego wrażenia, np. interesujące formatowanie kodu:

        if(session.find(Account.class, "manager") == null){
        Account manager = new Account();
        manager.setActive(true);
        manager.setEncrytedPassword(bCryptPasswordEncoder.encode("123"));
        manager.setUserName("manager");
        manager.setUserRole("ROLE_MANAGER");
        session.save(manager);
        }

Do twojego warsztatu i dbałości o szczegóły nie budzą też zaufania literówki ("encryted", "quanity").


Nie ma najmniejszego powodu, aby w CV pisać "email" przed swoim adresem mailowym, "imię i nazwisko" przed imieniem i nazwiskiem" ani "zdjęcie mojej głowy od przedniej strony" obok ewentualnego zdjęcia.

Pozostało 580 znaków

2019-02-27 11:54
3

Nie ma testów, a dla pracodawcy to bardzo ważne.


Jeśli chcesz pomocy, NIE pisz na priva, ale zadaj dobre pytanie na forum.

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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