Prośba Github/Code review.

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!

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 :)

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 ;)

3

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

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 :)

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

  2. 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))
  1. 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);
  1. 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ść.

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

  2. poczytaj o testach jednostkowych i Junit

  3. wstrzykiwanie przez konstruktor

  4. EntityManager lepsze od SessionFactory:
    https://stackoverflow.com/questions/5640778/hibernate-sessionfactory-vs-entitymanagerfactory

itd. ale na dzisiaj starczy :)

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

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.

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.

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").

3

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

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