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ść

0

Cieszę się, że już się czegoś przydatnego dowiedziałem o DI :)
Natomiast główna kwestia - taki github mogę podpiąć sobie pod cv czy raczej powinienem się wstydzić i ukrywać przed rekruterem/pracodawcą? Sama czystość kodu jako tako czy te moje repozytoria to jakieś potworki?

0

IMO ani jakoś szczególnie nie pomoże, ani nie zaszkodzi, tak naprawdę to jest nudna niczym flaki z olejem. Możesz wrzucić, przynajmniej pokażesz że znasz podstawy Springa. I dodaj logowanie z slf4j chociaż, trochę więcej kodu będzie.

2

@graczor93: zamiast zwykłego CRUDA możesz napisać coś umiarkowanie bardziej skomplikowanego. Np. API dla szkoły tańców latynoskich xD gdzie na początku będzie służyło ono do logowania czasu ile dany użytkownik (kursant) korzystał, liczenie hajsu od klienta i uwaga - załóż że sa różne typy płatności (np. inaczej jest jak ktoś wykupuje pojedyńcze zajęcia, inaczej jak ma multisport, inaczej jak ma karnet open itd). Zrób tak żeby można było dodać nową formę płatności bez naruszania liczenia hajsów ze starych. Możesz później dodac np. jakieś generowanie raportów z wyborem ecella albo PDFa.

A poza tym polecam skorzystać z VAVRa

0

@scibi92: Rzeczywiście może coś takiego wprowadzę w swoim nawet gym receptionist - jakieś zajęcia zumby i inne :D
W sumie ciężko mi było wybrać coś ciekawego na projekcik - mam koleżków co cwancykiem przepisują jakies proste tutoriale jak todo list czy borrower i wklepują jako swój projekt na github, wg nich działa. Kodzik jest prosty, ładny bo zjechany z tutoriala i wszystko bangla. Ja miałem olbrzymie opory żeby takie coś odwalić.
Myślałem też o wrzuceniu sobie prostego projektu utworzonego przy pomocy tdd - choćby nawet gra w statki lub kółko i krzyżyk

0

@scibi92: takie api to nadal praktycznie crud, lepiej coś co wymaga minimum algorytmiki chociażby serwis rozwiązujący nadesłane sudoku albo przetwarzający nadesłane zdjęcia według wybranego filtra, generalnie coś wymagającego więcej niż odczytaj/zapisz dane do bazy.

1

Może nadal to CRUD ale wymagający znajomości OOP i SOLID przynajmniej...

0

Ok poprawiłem chyba to o czym wspomnieliście mi tutaj. Jeżeli macie jeszcze jakieś uwagi to będę wdzięczny za wszelkie spostrzeżenia. Dziekuję z góry :)

0

Autowired, to zło. Po pierwsze, jak to testować? Po drugie jak masz dopisać 15ste Autowired, to zrobisz to, a jak będziesz miał dopisać 4 parametr do konstruktora, to może sobie zadasz pytanie, że jednak coś to jest nie tak.

0

Czyli jeżeli mam tutaj w controllerze dużo parametrów konstruktorze, przez te convertery itp. to lepiej przenieść to konwertowanie do serwisu? Znowu na jakimś blogu znalazłem w momencie pisania appki że dobrą praktyką byłoby konwertowanie już w kontrolerze i jestem nieco w kropce

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