Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spring Core] 김수민 미션 제출합니다. #265

Open
wants to merge 23 commits into
base: boyekim
Choose a base branch
from

Conversation

boyekim
Copy link

@boyekim boyekim commented May 27, 2024

이번 한 주도 고생 많으셨습니다!
Service를 분리하는 이유가 체감 된 과제였던 것 같습니다.
새로운 객체인 Time이 들어오면서 DTO를 더 분리하였습니다.
상당히 많은 수정을해서(Service, Request와 Response분리 등) 전체적으로 코드가 깔끔하게 작성되었는지, 역할의 분리가 잘 되었는지를 중점으로 봐주시면 감사하겠습니다

@che7371
Copy link

che7371 commented May 28, 2024

안녕하세요! 미션하시느라 정말 수고 많으셨습니다☺ 스프링에 대한 지식이 부족하여 리뷰 퀄이 낮을 수 있다는 점 양해 부탁드리겠습니다. 열심히 리뷰 해보도록 하겠습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 time, reservation, home 빼두신 거 좋은 선택인 것 같아요. 좀 더 세부적으로 나누시려고 빼신 것 같은데 맞을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HomeController 클래스 말씀하시는 것 맞을까요? 이외의 컨트롤러에서 RequestMapping을 써서 좀 더 깔끔하게 만들고자 단순히 페이지 뷰 이름만 반환하는 컨트롤러를 따로 분리하였습니다 ㅎㅎ

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TimeController도 요구사항에 맞춰서 잘 짜신 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 요구사항에 맞춰서 잘 동작하도록 잘 작성하신 것 같습니다.

public ResponseEntity<ReservationDTO> reservationAddController(@Valid @RequestBody ReservationDTO reservationDTO) {
ReservationDTO responseDTO = reservationRepository.reservationAdd(reservationDTO);
@PostMapping
public ResponseEntity<Reservation> reservationAddController(@Valid @RequestBody Reservation reservation) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Valid 어노테이션을 붙이신 이유가 뭔지 궁금해요! 이 어노테이션은 어떤 역할을 하나요? 저는 항상 @RequestBody만 붙였었는데 이건 처음 봐서요! 제가 스프링에 대해서는 아직 모르는 게 많아서 여쭤봅니다☺

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 요구사항 맞춰서 잘 하신 것 같습니당

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 전부 repository에서 service를 호출하셨네요. 굿입니다☺

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responseDTO도 만드셨군요! 저는 requestDTO만 생각했는데, 하나 배워갑니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 잘 고치신 것 같습니당

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service 클래스를 만들 후 여기서 repository를 호출해서 db 관련 처리하게 만드신 거 좋은 선택이신 것 같습니다. 구조에 대한 이해도 잘 하고 계신 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

감사합니다! 구조가 좀 더 복잡해지면서 repository에서 db의 조작을 하는 것보단 service를 도입하여 역할을 분리하는 것이 좋다고 생각했습니다.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 service 굿입니당☺

@che7371
Copy link

che7371 commented May 29, 2024

수정된 부분이 말씀하신 것처럼 많은데 그만큼 열심히 하신 게 느껴져요. 과제 요구사항을 완벽히 지키려고 노력한 느낌? 특히 service를 분리하는 이유가 체감이 되셨다고 하셨는데 그게 저한테까지 느껴져요. 미션을 진행할 수록 구조나 코드가 점점 발전하는 느낌이었습니다. 구조도 잘 짜인 것 같고, DTO도 더 분리하셔서 역할의 분리도 잘 된 것 같아요. 특히 DTO 부분은 저도 배워갑니다! 열심히 하신 게 정말 잘 느껴져서 저도 자극 받고 갑니다. 미션하신다고 수고 많으셨어요! ☺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants