Code review - wielka prośba

0

Hej,
Od kilku miesięcy jestem zatrudniony jako junior java developer. Trafiłem na to stanowisko po jedynie 2 miesiącach nauki Javy. Ogólnie jestem często chwalony za swoją pracę itp. Jednak dziubie sobie teraz jakieś swoje projekciki i brakuje mi code review. Czy byłby tu ktoś na tyle miły, żeby zerknąć w mój kod i podzielić się wskazówkami ? Znajduje się tam kilka repo - Zadanie rekrutacyjne to zadanie do mojej obecnej pracy - wiem że zabrakło tam gitignore itp., jednak no uczyłem się jedynie 2 miesiące programowania :)
https://github.com/JakubGraczyk?tab=repositories
Będę mocno zobowiązany. Bardzo zależy mi na rozwoju ukierunkowanym we właściwą stronę.
Pozdrawiam

@must - miałem ZadanieRekrutacyjne, które firma mi przekazała do wykonania. Zadanie wykonałem w ich odczuciu na tyle dobrze ,że dano mi szansę.

2

Na pewno nie wrzucamy plików Idei czy innego IDE .gitgnore sie kłania

0

ZadaniaRekrutacyjne:

                int change = 1;
        while (change > 0) {
            change = 0;

W javie do sterowania pętlą po prostu użyj boolean, int jako flaga true/false nie jest zbyt java.


public int convert(String return_number) throws OwnException {

        Integer number = null;
        try {
            number = Integer.valueOf(return_number);
        } catch (NumberFormatException nfe) {
            System.out.println("Exception - wrong number format");
            throw new OwnException();

        } catch (Exception e) {
            System.out.println("Different Exception");
        }
        return number;

    }

Drugi catch zjada wyjątek i zwraca null też powinieneś rzucić wyjątek. Dodatkowo ostatnio popularne jest rzucanie wyjątków typu runtime żeby pozbyć się masy try/catch.

    public Object[] findBiggestAmount(Person person)

Ten return type jest paskudny, zdefiniuj jakąś klasę która opisze Wynik.

GymReception / family docker:
Ciężko coś oceniać całość to kilka prostych serwisów.

0

@xxx_xx_x - Okej co do ZadanieRekrutacyjne - ja pisałem tam jedynie TESTY. Część, która nie jest testami została mi wysłana jako zadanie - metody do przetestowania. Przepraszam nie doprecyzowałem. A jakichś baboli nie widać na pierwszy rzut oka w families? Chodzi mi o to czy takie repo na gicie bardziej zachęci potencjalnego kolejnego pracodawcę do kontaktu czy odrzuci
@scibi92 - takl jak zaznaczylem w 1 poście wspomniałem, że mam świadomość że zabrakło tam gitignore.

1

Nie wiem po co tak naprawde service i serviceimpl osobno. Robienie interfejsu z jedną implementacją w przypadku serwisów nie ma za bardzo sensu. Zresztą one nie sa potrzebne tutaj bo to czysty CRUD. poza tym zwracanie encji przez kontrolery to nie jest najlepszy pomysł... Lepiej zwracać niemutowalne DTO

@graczor93 no nie, takie coś jak to:

@Autowired
    public FamilyRepository familyRepository;
    @Autowired
    private FatherRepository fatherRepository;
    @Autowired
    private ChildRepository childRepository;
    @Autowired
    private FamilyConverter familyConverter;
    @Autowired
    private FatherConverter fatherConverter;
    @Autowired
    private ChildConverter childConverter;
    @Autowired
    private ChildService childService;

To jakas tragedia okrutna. Słyszałes o czymś takim jak Dependency Injection?

0

Na pierwszy rzut oka nie widzę tam nic strasznego, ale zajmuje się głównie androidem, a serwisy piszę do swoich amatorskich projektów wiec nie chce za wiele w tym temacie się wypowiadać.

Lepiej wymyśl sobie jakiś ciekawy projekt, który pokaże że potrafisz klepać algorytmy, a nie tylko korzystać z frameworków.

0
scibi92 napisał(a):

Nie wiem po co tak naprawde service i serviceimpl osobno. Robienie interfejsu z jedną implementacją w przypadku serwisów nie ma za bardzo sensu. Zresztą one nie sa potrzebne tutaj bo to czysty CRUD. poza tym zwracanie encji przez kontrolery to nie jest najlepszy pomysł... Lepiej zwracać niemutowalne DTO

@graczor93 no nie, takie coś jak to:

@Autowired
  public FamilyRepository familyRepository;
  @Autowired
  private FatherRepository fatherRepository;
  @Autowired
  private ChildRepository childRepository;
  @Autowired
  private FamilyConverter familyConverter;
  @Autowired
  private FatherConverter fatherConverter;
  @Autowired
  private ChildConverter childConverter;
  @Autowired
  private ChildService childService;

To jakas tragedia okrutna. Słyszałes o czymś takim jak Dependency Injection?

Tak słyszałem. A więc jak to rozwiązać? Widziałem podobne twory w pracy w projekcie, w którym pracuję. I jak to najlepiej zrefaktoryzować?

1

@graczor93: jeśli takie twory masz w pracy to lepiej uciekać jak najszybciej.,

Otóz sprawa jest prosta:

@Service
public final class AwesomeService {

 private final AwesomeRepository awesomeRepository;

 public AwesomeService(AwesomeRepository awesomeRepository) {
  this.awesomeRepository = awesomeRepository;
}
0

@scibi92: okej, czyli wstrzyknąć wszystko przez konstruktor i będzie znacznie lepiej. No cóż mam projekt w Java EE gdzie właśnie są widoczne @Inject od góry do dołu. Co do zmiany pracy - po to własnie klepię sobie swój kodzik po godzinach i wrzucam go tu, aby zwiększyć swoje szansę. Chociaż proijekt w którym siedze, jest przejęty po innej firmie. Nie wiem dokładnie kto go tak popsuł

Ok, lepiej to wszystko pokasować z githuba i nie podawać linka np w CV czy przy aplikacjach na juniora takie rzeczy przejdą? Wiem, że kod nie jest doskonały. Ciężko uczyć mi się w pracy na "popsutym" kodzie projektu. Zależy mi na rozwoju. Będę wdzięczny też za takie podsumowanie ;)

0

Używanie @Autowired (pozdrawiam użytkownika) na konstruktorze ma jeszcze jedną zaletę. Wizualnie pokazuje kiedy klasa ma za dużo zależności i co za tym idzie prawdopodobnie za dużą odpowiedzialność. Dodając kolejne pole tylko aż tak tego nie czuć 

EDIT
no i testowalość

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