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

[4 - 9단계 - 방탈출 예약 관리] 알파카(최휘용) 미션 제출합니다. #160

Merged
merged 32 commits into from
Apr 30, 2024

Conversation

slimsha2dy
Copy link
Member

안녕하세요 썬, 알파카입니다!
오랜만에 다음 단계 리뷰를 부탁드리게 되었네요..ㅎㅎ

스프링을 거의 모르는 상태로 구현 위주로 미션을 진행하다 보니 아직은 질문이 없습니다.
최대한 미션 요구사항 위주로 완성해봤는데 개선할 점에 대해 리뷰 남겨주시면 열심히 공부하고 수정해보겠습니다.
남겨주시는 리뷰와 관련된 부분이나 추후에 제가 공부하는 부분에 대해 궁금한 점이 생기면 다시 남겨보겠습니다.
이번 리뷰도 잘 부탁드립니다!

Copy link

@syoun602 syoun602 left a comment

Choose a reason for hiding this comment

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

안녕하세요 알파카!

2단계 잘구현해주셨습니다 👏
스프링을 처음 접하신다고 하셔서 질문/학습 위주로 코멘트 남겨봤으니 확인 후 재요청 주세요!
반영 중 궁금한게 있다면 언제든 DM으로 질문 주셔도 됩니다~

README.md Outdated
@@ -14,10 +14,20 @@
- [X] : 예약 추가 API 구현한다.
- [X] : 예약 취소 API 구현한다.

## stpe7

Choose a reason for hiding this comment

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

Suggested change
## stpe7
## step7

Comment on lines 21 to 24
@Autowired
public ReservationRestController(ReservationService reservationService) {
this.reservationService = reservationService;
}

Choose a reason for hiding this comment

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

생성자 주입 방식을 사용해주셨네요 👍
학습해보면 좋을 키워드를 드릴테니 학습해보시고 공유해주세요!

  1. 스프링은 어떻게 new 없이 의존성을 주입할까요?
  2. 주입의 방식이 여러가지가 있는데 어떤 것들이 있고, 왜 생성자 방식이 권장될까요?
  3. 현재 @Autowired 어노테이션을 제거해도 동작할겁니다. 왜 가능할까요?

Copy link
Member Author

@slimsha2dy slimsha2dy Apr 26, 2024

Choose a reason for hiding this comment

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

  1. @Component어노테이션을 사용한 클래스들은 스프링 컨테이너에서 객체 빈을 생성해두고 이렇게 생성된 한 객체를 필요할 때 주입해서 사용하게 됩니다. 현재 코드에선 ReservationController에 사용된 @RestController@Component가 포함되어 있으므로 빈으로 등록되고, 컨트롤러에서 해당 빈을 new 없이 주입받을 수 있게 됩니다.
  2. @Autowired를 사용하면 주입을 할 수 있는데, 생성자에 사용하는 방식, 필드에 사용하는 방식, Setter에 사용하는 방식이 있습니다. 생성자 방식을 활용할 때의 장점은 여러 가지가 있습니다. 일단 필드에 final 키워드를 사용할 수 있게 되어 불변 객체로 선언할 수 있게 되고, 나머지 두 방식이 순환참조 문제를 런타임에 알게 되는 반면 생성자에 사용 시 컴파일 타임에 알 수 있게 됩니다. 또한 테스트 코드에서 생성자를 호출해 원하는 의존성을 주입할 수 있다는 테스트의 용이성도 장점인 것 같습니다.
  3. Spring 4.3 이상부터는 생성자가 하나인 경우 @Autowired를 사용하지 않아도 되는군요. 제 코드에서도 굳이 필요 없는 경우 따로 사용하지 않도록 수정했습니다!

혹시 제가 잘못 이해했거나 놓친 부분이 있을까요?

Choose a reason for hiding this comment

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

잘 학습해주셨네요 👍

}

@GetMapping("/reservations")
public List<ReservationResponse> reservations() {

Choose a reason for hiding this comment

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

모든 메서드 path에 /reservations가 붙어있는데, 중복이 제거 가능할 것 같아요~

hint: @RequestMapping

Copy link
Member Author

Choose a reason for hiding this comment

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

컨트롤러들에 @RequestMapping를 적용해 중복을 줄여봤습니다!

}

@PostMapping("/reservations")
public ResponseEntity<ReservationResponse> addReservationInfo(@RequestBody ReservationRequest reservationRequest) {

Choose a reason for hiding this comment

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

그냥 addReservation으로 작성해도 되지 않나요? 🤔

Comment on lines 30 to 31
String sql = "SELECT r.id AS reservation_id, r.name, r.date, t.id AS time_id, t.start_at "
+ "AS time_value FROM reservation AS r INNER JOIN reservation_time AS t ON r.time_id = t.id";

Choose a reason for hiding this comment

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

sql을 엔터를 활용해 가독성 좋게 분리해봅시다~ 또한 자바의 multiline string에 대해 알아보아요~

Comment on lines 79 to 88
RestAssured.given().log().all()
.when().delete("/reservations/1")
.then().log().all()
.statusCode(200);
.statusCode(204);

RestAssured.given().log().all()
.when().get("/reservations")
.then().log().all()
.statusCode(200)
.body("size()", is(0));

Choose a reason for hiding this comment

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

컨트롤러 테스트라면 취소가 성공적으로 됐다는 응답인 204만 확인해도 될 것 같아요.
그렇다면 정말 취소가 됐는지에 대한 테스트는 어디서 진행해야할까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

정말 취소가 됐는지에 대한 테스트는 직접 쿼리를 날리는 dao에서 테스트를 하는 것으로 충분해 보일 것 같네요. 컨트롤러 테스트에서는 응답만 확인하도록 수정했습니다!

}

public ReservationResponse addReservation(ReservationRequest reservationRequest) {
return new ReservationResponse(reservationDao.add(reservationRequest));

Choose a reason for hiding this comment

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

현재 request 객체가 controller로부터 dao까지 전 범위를 넘나들고 있는데, 의도한 사항일까요~?

Copy link
Member Author

@slimsha2dy slimsha2dy Apr 26, 2024

Choose a reason for hiding this comment

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

Reservation 객체를 db 테이블의 값과 완전히 대응시키려 하다보니 이렇게 되었습니다.
primary key인 id를 알기 위해선 테이블에 INSERT를 해야 하므로 이를 수행하는 dao에서 id를 얻은 후 Reservation 객체를 만들도록 구현하게 되었습니다.
이유는 비즈니스 로직이 추가되면 이를 처리할 때 Reservation 객체를 사용할텐데 혹시 id 값을 필요로 할 수도 있지 않을까 생각했기 때문인데 사실 제가 괜한 확장을 고려한 것인지에 대한 의문이 들기는 했습니다.
request 객체의 책임은 http 요청을 받는 것이므로 dao까지의 전달을 지양하고 계층 간 전달을 목적으로 하는 dto를 추가로 구현하거나, 차라리 Reservation 객체의 필드에서 id를 빼는 방법으로 해당 로직을 변경하는 편이 더 좋을까요?

Choose a reason for hiding this comment

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

Domain과 Entity를 동일시하고 있어서 혼란이 있는 것 같아요.
개인적으로 dto를 추가로 구성하는 것은 복잡도나 유지보수성 측면에서 불편할 것 같아요.
현재처럼 request를 모든 계층에서 사용하거나 말씀하신대로 id가 없는 reservation을 만드는 것도 방법인 것 같아요.
알파카가 어느 장점에 더 비중을 둘건 지를 결정하고 구현해보면 좋을 것 같습니다 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

dto는 비교적 수정이 많이 발생하는 클래스라고 알고 있었는데, 만약 request를 수정하게 됐을 때 모든 계층을 변경해야 한다면 비직관적이고 유지보수하기 힘든 코드인 것 같아요. 그렇다고 만약 Request dto를 컨트롤러에서만 사용하게 하고 도메인이 계층을 이동하게 된다면, id를 null로 초기화하는 부분이 발생하게 되고(db에 INSERT하기 전까지) setter를 열어야 하는데 이런 방향이 괜찮은지에 대해 감이 잘 오지 않네요. 일단 이 방법의 단점이 더 크다고 생각해 지금과 같은 구현으로 할 생각인데, 썬은 혹시 이에 대해 어떻게 생각하시나요? 🙇

Choose a reason for hiding this comment

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

우선 제 말이 정답이 아닌 의견임을 말씀드릴게요!
저라면 지금처럼 도메인=엔티티가 동일한 구조라면 requestDto를 컨트롤러/서비스 계층까지만 사용하고,
persistence layer에서는 도메인을 사용할 것 같아요.
이유는 간단합니다. 클라이언트에 대한 요청 또는 응답이 변경될 여지가 가장 많은데, 변경이 있을 때마다 모든 계층에서의 코드 수정이 이루어져야 하기 때문이죠. dto를 분리하는 것도 방법이긴 하겠지만, 앞선 코멘트에서 말씀드린 이유로 굳이 나누진 않을 것 같아요. (물론 엄격하게 각 계층을 분리하고 싶다면 말리진 않습니다)

말씀하신 setter 부분은 부생성자this(null, ...)를 활용해서 객체를 만들어볼 수 있어요👀
id를 필요로 한 경우가 정확히 어떤 상황을 염두하셨는지 모르겠으나, 서비스 레이어에서 null 체크를 진행해볼 수 있지 않을까요?

Comment on lines 17 to 18
@Repository
public class ReservationDao {

Choose a reason for hiding this comment

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

질문) Dao와 Repository는 어떤 차이가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 저는 거의 차이가 없다고 생각하고 해당 용어들을 사용했었어서 남겨주신 리뷰를 보고 한 번 찾아봤습니다.

  • Dao: 일부 종류의 데이터베이스나 기타 퍼시스턴스 매커니즘에 추상 인터페이스를 제공하는 객체
  • Repository: 엔티티에 의해 생성된 DB에 접근하는 메소드들을 사용하기 위한 인터페이스

Dao는 데이터에 접근하도록 DB접근 관련 로직을 모아둔 객체이고, Repotiroy는 엔티티 객체를 보관하고 관리하는 저장소라고 볼 수 있을 것 같은데, 제가 이해하기로는 개념에서의 차이가 조금은 있긴 하지만 실제로 사용할 때는 큰 차이가 없는 것처럼 느껴집니다.
실제로 현업에서의 스프링에서는 이 둘을 명확히 구분해서 사용하게 되는건지 궁금합니다.

Choose a reason for hiding this comment

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

저도 현업에서는 거의 jpa+repository를 사용했어서 아직까지 명확히 구분해서 사용한 적은 없는 것 같아요~
말씀하신대로 테이블과 1대1로 매핑되는 도메인이라면 그렇게까지 유의미하지 않을 수 있는데,
하나가 아닌 여러개의 테이블과 연관되는 작업이어도 여전히 dao일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dao가 테이블에 매핑되는 객체, Repository가 엔티티에 매핑되는 객체라고 이해했습니다. 만약 여러개의 테이블과 연관되는 작업이라면 Dao보다는 Repository라는 이름이 더 적합하다고 느껴지네요. 지금 ReservationDaoreserationreservation_time이라는 두 테이블에 접근하고 있으니 Repository라는 네이밍으로 수정했고, 일관성을 위해 ReservationTime또한 마찬가지로 수정했습니다!

Comment on lines 13 to 14
public ReservationRequest() {
}

Choose a reason for hiding this comment

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

기본 생성자가 왜 필요한가요? response에는 final을 붙이셨는데 여기는 왜 final을 붙이지 않으셨나요~?

Copy link
Member Author

Choose a reason for hiding this comment

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

@RequestBody로 객체를 받기 위해 기본 생성자와 Getter가 필요하다고 알고 있었어서 위와 같이 구현했었는데, 말씀을 듣고 빼봤는데도 잘 돌아가네요.. 그래서 제가 잘못 알고 있었나 확인하기 위해 TimeRequest의 기본 생성자도 뺀 후에 실행해보니 또 여기서는 문제가 발생하더라구요. 😢
그래서 해당 부분에 대해 찾아보니, 생성자가 1개이고 파라미터가 1개보다 많다면 기본 생성자가 없더라도 @RequestBody를 사용하는 데에 전혀 문제가 없다는 것을 알게 되었습니다. 하지만 만약 파라미터가 1개라면 Jackson이 바인딩하는 모드에 모호함을 주게 되어 제대로 동작하지 않게 되므로, 해당 생성자에 @JsonCreator 어노테이션을 붙여주거나 기본 생성자를 추가해야 한다는 사실을 알게 되었습니다.
그런데 만약 이후에 생성자가 추가되면 기본 생성자를 다시 추가해야 하는데, 이런 경우에 final을 떼어야 하는 것은 어쩔 수 없는 부분일까요..?

Choose a reason for hiding this comment

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

기본 생성자를 작성하는 편인데 습관적으로 작성하는 코드에 대해 고민을 해보시면 좋을 것 같아서 남겨보았습니다 😊
잘 찾아서 학습해주셨네요 👍👍

Comment on lines 13 to 15
time_id BIGINT, -- 컬럼 수정
PRIMARY KEY (id),
FOREIGN KEY (time_id) REFERENCES reservation_time (id) -- 외래키 추가

Choose a reason for hiding this comment

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

주석은 제거하는 방향으로 진행해주세요~!

Copy link

@syoun602 syoun602 left a comment

Choose a reason for hiding this comment

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

안녕하세요 알파카!

코멘트에 대해 열심히 학습해주셨네요 💯
추가적으로 더 남겨보았는데 확인 후 재요청 주세요 :)

import org.springframework.web.bind.annotation.RestController;
import roomescape.dto.ReservationRequest;
import roomescape.dto.ReservationResponse;
import roomescape.service.ReservationService;

@RestController
public class ReservationRestController {
@RequestMapping(value = "/reservations")

Choose a reason for hiding this comment

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

TimeController는 파라미터명을 작성하지 않으셨던데, 여기도 똑같이 일관성을 지켜볼까요?

Comment on lines 8 to 9
private LocalDate date;
private LocalTime time;
private ReservationTime time;

Choose a reason for hiding this comment

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

현재 구현사항에서 final이 아닐 이유가 없어보입니다 👀

import roomescape.dto.TimeRequest;
import roomescape.dto.TimeResponse;

@Service

Choose a reason for hiding this comment

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

학습차 질문드립니다!
@Component, @Controller, @Service 들은 어떤 차이가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • @Component: 스프링 컨테이너 빈으로 관리된다. 컴포넌트 스캔이 가능해진다.
  • @Controller: 스프링이 클라이언트의 요청을 처리하기 위해 해당하는 Controller를 찾아야 하는데 이 때 해당 어노테이션이 있는 클래스들을 탐색한다.
  • @Service: 실제로는 @Component와 기능적으로 차이점이 없지만 서비스 계층임을 나타내기 위해 사용한다.
  • @Repository: 스프링에서 지원하지 않는 특정 예외를 Spring Exception으로 전환, 예외 발생 시 Unchecked Exception을 DataAccessException으로 변경하여 처리. 나머지는 @Component와 동일.

컨트롤러와 레포지토리는 기능적으로 컴포넌트와 차이가 있지만 서비스는 없네요. 다만 어떤 역할을 하는지 명시적으로 드러낼 수 있다는 장점이 있기 때문에 굳이 @Service 대신 @Component를 사용할 필요는 없어보이네요. 혹시 제가 잘못 알고 있거나 놓친 부분이 있을까요?

Choose a reason for hiding this comment

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

잘 학습해주셨어요! 스프링은 내용이 방대하기 때문에 이렇게 사용하시면서 하나씩 학습해가시는 것을 추천드립니다! 어노테이션 안에는 어떤 어노테이션을 추가로 구성해두었는지, 어떤 차이가 있는지를 살펴보면 좋을 것 같아요 😊

Comment on lines 18 to 19
return reservationTimeDao.findAll().stream()
.map(TimeResponse::new)

Choose a reason for hiding this comment

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

한 줄에 점 하나 👀

Suggested change
return reservationTimeDao.findAll().stream()
.map(TimeResponse::new)
return reservationTimeDao.findAll()
.stream()
.map(TimeResponse::new)

}

public TimeResponse addReservationTime(TimeRequest timeRequest) {
return new TimeResponse(reservationTimeDao.add(timeRequest));

Choose a reason for hiding this comment

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

변수로 추출하면 가독성이 더 좋을 것 같네요 🙂

Comment on lines 51 to 54
Map<String, Object> reservation = new HashMap<>();
reservation.put("name", "브라운");
reservation.put("date", "2023-08-05");
reservation.put("timeId", 1);

Choose a reason for hiding this comment

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

Map.of() 를 활용해볼까요?

Comment on lines 21 to 24
@Autowired
public ReservationRestController(ReservationService reservationService) {
this.reservationService = reservationService;
}

Choose a reason for hiding this comment

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

잘 학습해주셨네요 👍

}

public ReservationResponse addReservation(ReservationRequest reservationRequest) {
return new ReservationResponse(reservationDao.add(reservationRequest));

Choose a reason for hiding this comment

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

Domain과 Entity를 동일시하고 있어서 혼란이 있는 것 같아요.
개인적으로 dto를 추가로 구성하는 것은 복잡도나 유지보수성 측면에서 불편할 것 같아요.
현재처럼 request를 모든 계층에서 사용하거나 말씀하신대로 id가 없는 reservation을 만드는 것도 방법인 것 같아요.
알파카가 어느 장점에 더 비중을 둘건 지를 결정하고 구현해보면 좋을 것 같습니다 😊

Comment on lines 17 to 18
@Repository
public class ReservationDao {

Choose a reason for hiding this comment

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

저도 현업에서는 거의 jpa+repository를 사용했어서 아직까지 명확히 구분해서 사용한 적은 없는 것 같아요~
말씀하신대로 테이블과 1대1로 매핑되는 도메인이라면 그렇게까지 유의미하지 않을 수 있는데,
하나가 아닌 여러개의 테이블과 연관되는 작업이어도 여전히 dao일까요?

Comment on lines 13 to 14
public ReservationRequest() {
}

Choose a reason for hiding this comment

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

기본 생성자를 작성하는 편인데 습관적으로 작성하는 코드에 대해 고민을 해보시면 좋을 것 같아서 남겨보았습니다 😊
잘 찾아서 학습해주셨네요 👍👍

Copy link

@syoun602 syoun602 left a comment

Choose a reason for hiding this comment

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

안녕하세요 알파카 👋

리뷰 반영이나 질문들도 잘 학습해서 반영해주셨네요 👍👍
방탈출 예약 관리 미션 잘 진행해주셔서 이만 머지하겠습니다!
다음 미션도 화이팅 하세요 💪

}

public ReservationResponse addReservation(ReservationRequest reservationRequest) {
return new ReservationResponse(reservationDao.add(reservationRequest));

Choose a reason for hiding this comment

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

우선 제 말이 정답이 아닌 의견임을 말씀드릴게요!
저라면 지금처럼 도메인=엔티티가 동일한 구조라면 requestDto를 컨트롤러/서비스 계층까지만 사용하고,
persistence layer에서는 도메인을 사용할 것 같아요.
이유는 간단합니다. 클라이언트에 대한 요청 또는 응답이 변경될 여지가 가장 많은데, 변경이 있을 때마다 모든 계층에서의 코드 수정이 이루어져야 하기 때문이죠. dto를 분리하는 것도 방법이긴 하겠지만, 앞선 코멘트에서 말씀드린 이유로 굳이 나누진 않을 것 같아요. (물론 엄격하게 각 계층을 분리하고 싶다면 말리진 않습니다)

말씀하신 setter 부분은 부생성자this(null, ...)를 활용해서 객체를 만들어볼 수 있어요👀
id를 필요로 한 경우가 정확히 어떤 상황을 염두하셨는지 모르겠으나, 서비스 레이어에서 null 체크를 진행해볼 수 있지 않을까요?

import roomescape.dto.TimeRequest;
import roomescape.dto.TimeResponse;

@Service

Choose a reason for hiding this comment

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

잘 학습해주셨어요! 스프링은 내용이 방대하기 때문에 이렇게 사용하시면서 하나씩 학습해가시는 것을 추천드립니다! 어노테이션 안에는 어떤 어노테이션을 추가로 구성해두었는지, 어떤 차이가 있는지를 살펴보면 좋을 것 같아요 😊

@syoun602 syoun602 merged commit bf7cdc8 into woowacourse:slimsha2dy Apr 30, 2024
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