-
Notifications
You must be signed in to change notification settings - Fork 84
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단계 방탈출 예약 관리] 페드로(류형욱) 미션 제출합니다. #172
Conversation
- SELECT COUNT(*) -> SELECT * LIMIT 1
- @beforeeach 로 구현하던 테스트 격리를 @SQL 을 활용하도록 변경
- Controller -- dto --> Service - Service -- domain --> Dao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 페드로! 리뷰어 웨지입니다!
4~9단계 잘 진행해주셨네요 ㅎㅎ 전체적인 평은 괜찮다고 생각합니다
몇가지 리뷰 남겼으니 확인해주세요!
감사합니다.
@@ -1,4 +1,4 @@ | |||
package roomescape.controller.exception; | |||
package roomescape.exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(사소하지만) 이제 ExceptionHandler 라는 이름을 부여해도 되겠어요!
ControllerAdvice는 기술을 채택한거고, 해당 객체의 역할은 예외처리기 니까요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
패키지를 옮길 때 클래스명도 같이 수정했어야 했는데 놓쳤던 부분이네요! 반영했습니다.
import java.time.LocalTime; | ||
import roomescape.domain.ReservationTime; | ||
|
||
public record ReservationTimeAddRequest( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reservation의 경우 ReservationRequest라고 이름 지었으니까, 컨벤션을 addRequest 나 Request 중 하나로 맞춰주면 좋겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reservation
에는 add
를 붙이지 않았던 이유는 '예약 요청' 이라는 단어에 '예약의 추가해 줘' 라는 의미가 내포돼어 있다고 생각했기 때문이였어요.
반면 ReservationTimeRequest
와 같이 쓸 경우 등록된 '예약 시간'에 대한 정보를 요청하는 것인지, '예약 시간'을 추가해 달라는 것인지 모호할 것 같아서 이곳에만 add
를 별도로 사용했었는데요,
웨지의 말씀을 듣고 생각해 보니 프로젝트 전반적으로 컨벤션을 통일하는 것이 더 좋을 것 같아 모두 add를 붙이는 쪽으로 변경해 두었습니다🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 ㅎㅎ 보통 프로젝트는 여러 사람이 함께 작업하게 되니까요, 비슷한 역할을 하는 객체들은 네이밍을 통일하는게 같이 개발하는 모두에게 좋습니다
(통일되어 있지 않으면 중구난방됨 - add create plus inset save 등등..)
if (!timeService.checkReservationTimeExist(id)) { | ||
return ResponseEntity.noContent().build(); | ||
} | ||
timeService.removeReservationTime(id); | ||
return ResponseEntity.ok().build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클라이언트가 204 나 200으로 분기를 쳐야하는 상황이 아니라면 불필요한 처리라고 생각해요~ 한 가지만 존재해도 될거같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 가지로 통합하기 위해서는 '요청한 id의 존재 여부와 관계없이 서버에는 이제 그 id가 없어' 라는 의미를 살릴 수 있는 204
가 좀 더 적절할 것 같아 그렇게 수정해 두었습니다.
.usingColumns("name", "date", "time_id"); | ||
} | ||
|
||
private final RowMapper<Reservation> rowMapper = (rs, rowNum) -> new Reservation( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반복되는 rowMapper를 추출해주신것 좋네요 👍
하지만 매번 인스턴스로 생성될 필요가 없는 객체니 static으로 처리하셔도 되겠습니다
return new Reservation( | ||
key.longValue(), reservation.getName(), | ||
reservation.getDate(), reservationTime | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(사소함) 결국 코드는 사람이 보는거니까 리포트를 쓸 때 정리하듯이, 가능하면 정렬 등 을 이쁘게 유지하는 습관이 있으면 좋습니다 (가독성 측면)
return new Reservation( | |
key.longValue(), reservation.getName(), | |
reservation.getDate(), reservationTime | |
); | |
return new Reservation( | |
key.longValue(), | |
reservation.getName(), | |
reservation.getDate(), | |
reservationTime | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수정했습니다!
이것도 사소한 질문이긴 한데요, 지금까지 저는 한 라인이 너무 길어지지만 않는다면 인자 2개나 3개 단위로 개행을 하거나 개행 단위를 적당히 조절해서 인자 전달 부분의 폭을 통일시키는 방향으로 작성했었는데 웨지는 한 라인에 인자 하나만 있는 것을 선호하시나요?
new SomeObj(
id, name, age,
var.someLongMethodName(),
var2.nextLongMethodName(),
lastParam
);
new SomeObj(
id,
name,
age,
var.someLongMethodName(),
var2.nextLongMethodName(),
lastParam
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 제시해주신 것중엔 아무래도 위에가 더 이뻐보이긴 하네요 ㅋㅋ
그래도 저는 기본적으로 line by line을 선호합니다 (개당 필드 하나가 들어간다는 의미가 됨)
당장은 2자, 3자지만 var에 someLongMethodName 메서드를 호출한 것처럼 메서드 호출 등으로 변형되면 새롭게 개행해주어야 한다는 소요가 생기기도 하고요
하지만 본문에서 언급했듯 (사소) 합니다 ㅎ
그리고 저는 필드가 많아지는 상황이 되면 builder 패턴 등을 선호하기 때문에 line by line 이 자연스럽게 되기도 하고요
public List<Reservation> findAll() { | ||
String sql = """ | ||
SELECT | ||
r.id as reservation_id,\s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\s 처리는 필요했나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쿼리문은 ;
단위로 라인을 구분하므로 이스케이프가 필요 없다고 생각했었지만, 최초 코드를 작성했을 때 IDE에서 이스케이프가 없다고 경고문을 띄우는 것이 보기 싫어서 다 붙여줬었어요.
지금은 다시 \s
를 지워 보니 경고가 안 뜨네요🤦🏻♂️ 아마 처음 작성했을 때 인텔리제이에서 SQL을 파싱할 데이터소스가 제대로 구성이 안 되어 있었던 것 같아요. 모두 제거했습니다!
import roomescape.domain.Reservation; | ||
|
||
public record ReservationResponse(Long id, String name, LocalDate date, LocalTime time) { | ||
public record ReservationResponse(Long id, String name, LocalDate date, ReservationTimeResponse time) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR에 적어주신 고민에 대한 답변입니다!
1 해당 DTO를 인스턴스화 하는 쪽에 둔다.(Request는 컨트롤러에, Response는 서비스에)
2 결국 Controller의 응답/요청으로 볼 수 있으므로 컨트롤러 패키지 내에 둔다.
3 컨트롤러는 서비스 패키지에 접근할 수 있지만, 그 반대는 패키지 의존 관계가 맞지 않으므로(service -> controller) 서비스 내에 둔다.
1번은 고려대상은 아닙니다. '인스턴스화"는 해당 객체를 의존 가능한 곳 어디서든 할 수 있습니다. 지금 당장 인스턴스화 되는 곳도 중요하지만, 미래에 인스턴스화 될 가능성까지 고려해야합니다. 그러니 "현재 인스턴스화 되는 곳" 보다는 다른 관점에서 설계할 필요가 있습니다
"객체가 있어야 할 곳"에 있게 설계할 필요가 있다고 생각합니다. Dto의 성격을 생각해보면 '어딘가'에서 '어딘가'로 데이터를 전달해주기 위한 객체입니다.
2에서 말씀하셨듯 컨트롤러의 요청/응답 객체라고 본다면 이 객체는 여기에 있는게 맞습니다. 이 객체의 정의는 클라이언트가 던지는 데이터 타입과 필드를 결정하는 객체입니다.
저희는 웹의 요청을 받고 응답을 내려주어야 하니 이 객체는 필수적입니다
그 다음은 서비스 입장에서 보겠습니다
말씀하신 것처럼 패키지간의 의존흐름이 존재합니다. 서비스가 컨트롤러 패키지에 의존하는건 장기적으로 봤을 때 같은 역할이지만 클라이언트가 서비스가 다른 여러 컨트롤러와의 의존관계를 맺게 될 수 있기 때문에 취약합니다. (컨트롤러가 추가될때마다 서비스를 수정해야함)
기능은 구현해야하니까 그러면 서비스가 Dto를 제공할지 말지 결정해야합니다. 지금은 도메인 객체 구조가 간단하므로 도메인 객체만 두고 컨트롤러가 컨버팅해서 사용해도 무방합니다. 하지만 예약 객체에 필드가 12개가 되고, 이 중 2개 필드를 수정해야하는 API라면 컨트롤러가 항상 도메인 객체를 호출하는 게 불편한 구조가 될 수 있습니다.
이런 걸 생각해보면 서비스(도메인) 입장에서 생성Dto를 제공한다는 생각이 가능해집니다. 그리고 서비스가 제공하는 dto 이므로 이 dto의 위치는 서비스일겁니다.
그러면 2중의 Dto(웹 -> 컨트롤러 Dto, 컨트롤러 -> 서비스 dto)가 생기는데요, 현재 구조에선 이 2개의 객체가 다르지 않을겁니다. 실제로 유지보수를 진행할 때, controller dto를 수정할 때마다 service dto를 수정해야 할겁니다. 이런 불편을 겪다보면 이 둘이 왜 분리되어 있는지 고민이 들기도 합니다.
그런데 2번에 의해 컨트롤러 dto를 함께 사용하는 것은 불가능하므로, 여기서 서비스 dto를 컨트롤러가 같이 쓰는 방향도 생각해볼 수 있습니다.
이런 사고흐름을 가진다면 서비스 dto만 두고 컨트롤러 dto는 제거하는 방향으로도 갈 수 있습니다.
(이렇게 하라는 건 아닙니다. 고민하신 부분에 대한 제 사고흐름만 따라가봤습니다)
보시면 "틀린" 선택 결정은 '서비스가 컨트롤러 Dto에 의존한다' 정도만 있고 나머지는 다 선택입니다. (장단이 있음)
틀리다고 한 이유는 선택의 장점이 없기 때문입니다. 이런 것들은 선택지에서 배제하면 됩니다.
선택을 하실 때 완벽한 선택을 하려고 너무 많은 에너지를 쓸 필요는 없습니다.
코드는 쉽게 수정할 수 있으므로, 만약 선택이 어렵다면 아무거나 고르고 시간을 두고 피드백을 보는 게 낫습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 장문의 피드백 감사합니다😊 이제 조금 생각 정리가 된 것 같아요!
현재 단계의 수준에서 두 DTO를 분리할 필요까지는 없어 보이고(실제로 분리했다가 합쳤습니다🥲), 결국 재활용을 해야 할 것이라면 '내가 조금 어색하게 느끼기' vs '패키지 간 의존 흐름 안 맞추기' 중에 선택해야 한다면 결국 전자를 선택하는게 맞다고 판단했어요. 그 결과로 DTO 패키지의 위치를 서비스의 하위로 변경했습니다!
import roomescape.domain.Reservation; | ||
|
||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE) | ||
@Sql(value = "classpath:data-reset.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_METHOD) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SQL관련 질문 >
여러 방법이 있습니다!
SQL을 활용하신 것도 방법입니다. (제법 쓰입니다) 어치피 SQL에 의존적일 수 밖에 없는 Dao를 활용하는 상황이니까 너무 의존적이라고만 볼 순 없습니다.
그리고 철학 이야기를 할 때 중요한건 "대안이 있느냐"입니다. 개발적으로 불편하더라도 대안이 없으면 주장을 유연하게 조절하는 것도 중요합니다.
일단 sql은 되는 방법인데, 의존적이지 않게 할 수 있는 방법이 없으면 이 '되는 방법'이 가장 좋은 선택지입니다.
다른 방법을 제시해드리면 DB에 넣을 Fixture 객체를 테스트 패키지에 정의하고 직접 생성하는 겁니다. (어플리케이션에서 인스턴스화 하고 dao.save 부터 하기)
(현업에선 이 Fixture도 유지보수해야하는데 잘 안 되는 경우가 많습니다. 어떻게든 유지보수가 쉽게 가능하게 하려고 오만 방법을 다 쓰는데요, 전 그냥 운영환경 요청객체 Json 따와서 테스트 패키지에 .json file 째로 박고 ObjectMapper로 객체 파싱해서 씁니다. 도메인 객체가 필드가 50개씩 되면 일일이 fixture나 쿼리로 제어하기가 참 힘듭니다 ㅋㅋ)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 방법을 제시해드리면 DB에 넣을 Fixture 객체를 테스트 패키지에 정의하고 직접 생성하는 겁니다. (어플리케이션에서 인스턴스화 하고 dao.save 부터 하기)
이것도 생각해 봤었는데, dao.save()
를 테스트해야 할 테스트 클래스에서 .save()
를 사용하는 상황을 최대한 막기 위해서 독립된 쿼리문으로 먼저 밀어넣고 테스트하는 지금과 같은 방식을 선택했습니다🙂
전 그냥 운영환경 요청객체 Json 따와서 테스트 패키지에 .json file 째로 박고 ObjectMapper로 객체 파싱해서 씁니다.
사실 간단하기로는 제일 간단한 방법 같아요ㅋㅋㅋ 말씀하신대로 필드가 늘어났을때는 테스트를 위한 객체를 하나하나 다 관리하는것도 공수가 너무 크겠네요🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 테스트의 유지보수성도 생각보다 중요한 과업입니다.
남이 짠 테스트는 보수하기가 너무 어렵기 때문에, 만약 "테스트 통과 못하면 배포 못함" 등의 제약이 있는데 갑자기 남이 짠 테스트가 터지면..
주석처리 되거나 삭제처리 됩니다 😇
- ControllerExceptionHandler -> ExceptionHandler
reservationRequest -> ReservationAddRequest
- boolean isExist(Long id) 를 테스트 코드 내에서 정의하도록 변경
- 쿼리문의 불필요 이스케이프 문자 제거
- controller/dto -> service/dto
피드백 주신 부분 반영하여 다시 리뷰 요청 드렸습니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 페드로! 리뷰어 웨지입니다.
리뷰 반영 잘 해주셨네요 👍
이번 스텝은 여기서 머지하겠습니다!
수고하셨습니다 :)
public class ControllerExceptionHandler { | ||
@ExceptionHandler(IllegalArgumentException.class) | ||
public class ExceptionHandler { | ||
@org.springframework.web.bind.annotation.ExceptionHandler(IllegalArgumentException.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아이고 이거 때문에 객체명을 바꿔야겠네요 ㅋㅋ
(ReservationExeptionHandler 라든지 ApiExceptionHandler 라든지..)
네임 스페이스 충돌이 일어나고 있는데 좀 보기 싫으니.. (그렇다고 ExceptionHandler를 꼭 고수해야 할 필요도 없으니..)
안녕하세요 웨지! 계층 분리 후 각 계층간 정보 전달을 어떻게 할 것인지에 대한 고민이 길어지다 보니 리뷰 요청이 늦어졌네요😅
1~3단계 진행 당시에는 도메인 객체의
id
필드가 nullable 한 상황 자체가 문제가 된다고 생각했던 것과 더불어, Controller -> Service -> Dao 로의 데이터 흐름에서 각 계층간 의존성을 걷어내기 위해 분리할 수 있는 DTO는 전부 분리하여 사용했었는데요.미션을 진행하다 보니 동일한 구조의 다른 객체가 너무 많아지고, 그 객체들의 이름을 직관적으로 지을 수 없어 코드 작성 시 매번 찾아봐야 하는 불편이 생겼었습니다. 이뿐만 아니라, 동일한 형태의 데이터를 다루고 있음에도 불구하고 매번 형태를 변환해 줘야 하고, 변환 로직도 여러 곳에 분산되어 있어 한 눈이 구조를 파악하기 어렵다는 생각이 들었어요.
결론적으로 현재 상황에서는
null
비허용과 계층간 DTO를 분리하여 얻을 수 있는 이득보다 잃는 게 더 많다고 판단했습니다.여러 고민을 거쳐 생성 요청 처리 흐름에 한해
id
필드에null
을 허용하고 Service에서는 컨트롤러와 동일한 형태로 요청과 응답을 처리하도록 변경했어요. Dao 단에서는 INSERT 시 null id를 가지는 도메인 객체를 받고, SELECT 시 도메인 객체를 응답하도록 했습니다.대신, DTO -> Domain 으로의 변환은 원래 서비스의 책임이 아니므로, 응답/요청 스펙이 변경되었을 때 서비스의 코드가 아닌 DTO쪽을 변경할 수 있도록 DTO 내부에 변환 로직을 두고 서비스에서 이를 호출하도록 했어요. 응답/요청 형태의 작은 변경이 서비스의 수정을 야기하지 않도록 할 수 있는 나름의 절충안이라고 생각했습니다.
물론, 차후 분리의 필요성이 생기면 controller -> service 로 전달하는 DTO를 분리할 것 같아요. 그때 분리를 진행한다면 분리의 이유가 좀더 명확해지고, 클래스를 명명할 때도 해당 이유를 기반으로 보다 직관적인 명명이 가능할 것으로 기대됩니다.
이전 PR에서 웨지가 말씀해주셨던 패키지 구조에 대해서도 나름의 생각과 질문을 코멘트로 남겨 두었는데요, 이번 단계를 진행하면서 여러 클래스들이 추가되었고, 아직 답을 찾지는 못한 것 같아요. 현재 구조와 같이 하나의 ResponseDTO/RequestDTO 가 컨트롤러와 서비스단에서 모두 사용될 때, 웨지는 저 DTO들의 올바른 패키지 위치가 어디라고 생각하시나요?
추가로, 현재 컨트롤러와 서비스의 테스트는
@Sql
을 통해 테스트 메서드 간 격리를 구현해 두었는데요, 테스트코드를 작성하다 보니 너무 (테스트 클래스 외부에 정의된) 쿼리 파일에 의존적인 테스트가 되는 것이 아닌가 하는 생각이 들었어요. 예를 들어 아래와 같은 테스트는data-test.sql
에 예약 시간을 두 개 INSERT 하는 쿼리가 있다는 사실에 의존하고 있어요.또 한편으로는 원래
@BeforeEach
에서 하던 작업들을 보다 용이한 관리를 위해sql
파일로 분리한 것이라 크게 신경 쓸 필요가 없나? 라는 생각도 들기도 했구요.테스트를 위한 좀 더 좋은 방법이 있을지 궁금합니다!
이번 리뷰도 잘 부탁드려요👍