-
Notifications
You must be signed in to change notification settings - Fork 97
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
[2단계 - 경로 조회 기능] 테오(최우성) 미션 제출합니다. #161
Conversation
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단계 미션도 빠르게 잘 구현해 주셨네요 👍
개선할 포인트가 많지는 않지만 짚고 넘어가면 좋을 점들이 보여서 코멘트 남겨 두었습니다.
리뷰 반영해 보시면서 궁금한 점 있으면 언제든지 DM 주세요~
@@ -1,6 +1,6 @@ | |||
package subway.application.repository; | |||
package subway.application.port; |
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.
core, port를 굳이 나누기 보다는 domain 안에 model, repository, graph를 나누어놓으면 어떨까요?
model에는 Line, Station 등 일반 도메인 객체가 들어가고 repository는 infrastructure-repository의 인터페이스, graph는 infrastructure-graph의 인터페이스가 들어갑니다.
가능한 패키지는 너무 depth를 깊게 두지 않는 것이 좋습니다!
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.
감사합니다~
저는 core, port 구조를 통해 외부적/내부적 관점을 쉽게 드러낼 수 있다고 생각했었는데요,
말씀해주신 것처럼 depth가 너무 깊어지기도 하고, 용어가 추상적이라 직관적으로 이해하기 힘들다는 단점이 있을 것 같긴 합니다.
다만 궁금한 것이.. model, repository, graph, service 처럼 패키지를 구성하는 경우 패키지 자체에서 구조를 알아내기가 어려울 것 같다는 생각이 들어요.
위와 같은 그림이 될텐데, 만약 core, port의 개념이 존재하지 않고 평면적으로 패키지를 구성한다면 무엇이 무엇을 의존하는지 쉽게 파악하기가 힘들 것 같아요.
따라서 저는 클래스명, 변수명처럼 패키지 구조도 복잡하더라도 의도를 잘 드러낼 수 있다면 괜찮다는 입장인데 이에 대해 제이온은 어떻게 생각하시는지 궁금합니다..! 🧐
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.
개인적으로 depth가 깊을수록 클래스 네비게이션이 번거롭기때문에 가능한 적은 depth에서 시작하여 꼭 필요한 것이 아니면 패키지를 잘 두지는 않는 편입니다. 또한 도메인 패키지 안에 model, repository, graph(혹은 좀더 추상화한 네이밍)를 두고, service 패키지는 별개로 두는 것이기 때문에 구조를 알아내기 괜찮지 않을까요??
다만 어디까지나 패키지는 개인 혹은 팀의 성향이 강하기 때문에 지금 유지하셔도 좋고 구조 변경은 선택에 맡기겠습니다 ㅎㅎ
public class DeleteStationCommand extends SelfValidating<DeleteStationCommand> { | ||
|
||
@NotNull | ||
private final Long lineId; | ||
@NotNull | ||
private final Long stationId; |
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.
presentation의 dto와 application 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.
분리했을 때의 장점
- 서비스가 표현 계층의 컨텍스트에 묶이지 않게 됩니다. 즉 보다 서비스를 범용적으로 사용할 수 있게 되고(서비스 계층에서 표현 계층에 대한 어떠한 가정을 하지도 않으므로) 이는 서비스의 재사용 가능성을 의미합니다.
- 표현 계층의 변경 여파가 서비스로에 직접적으로 전파되지 않습니다. request DTO를 그대로 받는 경우 양방향 의존성이 생긴다고 볼 수 있는데(service가 표현 계층의 DTO를 의존하므로) 이런 경우 장기적으로 보았을 때 유지보수하기 좋지 않은 코드가 될 확률이 높다고 생각합니다.
- 오용 가능성이 줄어듭니다. DTO 자체만으로도 의미를 가지게 되었기 때문에 무엇을 전달해야 하는지, 무엇을 받는지 보다 명확해진다고 생각합니다.
분리했을 때의 단점
- 객체 관리 비용이 비쌉니다. 최악의 경우 메소드마다 2개의 DTO(입력 모델, 출력 모델)을 사용하는데 프로젝트 규모가 커지는 경우 관리 포인트가 무지막지하게 늘어나게 될 수 있습니다.
- 매핑 비용도 비쌉니다. 현재 핸들러 메소드에서 requestDTO -> serviceDTO, serviceDTO -> requestDTO로 가공을 두 번 해주고 있는데, 이런 매핑 작업도 상당히 번거롭다고 생각됩니다.
저는 분리했을 때의 장점이 더 명확하다고 판단이 되었기 때문에 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.
말씀하셨듯이 객체 관리 비용과 매핑 비용이 생각보다 크다보니 처음에는 이들을 분리하지 않는 편입니다. 또한 무지성으로 작성하면 presentation의 DTO를 거의 application의 DTO와 동일하게 복붙하여 별 의미없이 사용하게 될 수도 있습니다.
그래서 저 같은 경우는 처음에는 DTO를 일원화하다가 presentation과 application이 서로 복잡해져서 분리할 필요성이 생기면 그때 선택적으로 해 봅니다!
물론 지금은 연습 과정이므로 극한으로 분리해보는 것도 좋은 연습이 됩니다 ㅎㅎ
public abstract class SelfValidating<T> { | ||
|
||
private Validator validator; | ||
|
||
public SelfValidating() { | ||
ValidatorFactory factory = Validation.buildDefaultValidatorFactory(); | ||
validator = factory.getValidator(); | ||
} | ||
|
||
protected void validateSelf() { | ||
Set<ConstraintViolation<T>> violations = validator.validate((T) this); | ||
if (!violations.isEmpty()) { | ||
throw new ConstraintViolationException(violations); | ||
} | ||
} | ||
} |
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.
파라미터에 @Valid
를 붙이는 방식이 아닌 별도로 Validator를 구현하신 이유가 있나요?
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.
딱히 큰 이유는 없었는데요..!
생각해보니 말씀하신대로 @Valid
어노테이션을 사용하는게
서비스 코드만 보았을 때 검증이 된 정보가 넘어오는구나
라는 확신이 들 것 같아 좋아보입니다.
변경하도록 하겠습니다! 👍
+++
찾아보니 @Valid
는 ArgumentResolver에 의해 작동이 되는 것이라, 컨트롤러가 아닌 다른 빈에서 같은 기능을 사용하려면 AOP를 사용하는@Validated
까지 써야 한다고 하네요. 따라서 해당 어노테이션까지 추가해주었습니다!
소스 : 링크
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.
아하 보통 @Valid
만 가지고 프레젠테이션에서 DTO를 검증하다보니 제가 실수한 부분이 있었네요!
테오께서 스스로 잘 고쳐주셨네요 ㅎㅎ 훌륭합니다.
src/main/java/subway/presentation/support/LoggerInterceptor.java
Outdated
Show resolved
Hide resolved
import subway.presentation.dto.StationEnrollRequest; | ||
import subway.presentation.dto.StationResponse; | ||
|
||
import java.net.URI; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
@RestController | ||
@RequestMapping("/subway") |
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.
여기는 왜 lines가 아닌 subway인가요?
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.
subway
라는 리소스가 실제 존재하는 자원은 아니지만
클라이언트 입장에서는 노선도 반환, 노선에 역 추가 같은 기능이 subway
라는 리소스로 식별되어도 큰 문제는 없을 것 같아 위처럼 구성했는데요!
사실 이미 /lines
라는 path가 사용중이어서 그런 것도 있는데 기존 lines
를 /line-property
로 바꾸고 /subway
를 /lines
로 사용하는 편이 나을까요?
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.
저는 Controller가 다르다고 path를 꼭 구분할 필요는 없다고 생각합니다~
Line이나 LineProperty나 세부 역할을 구분하기 위해 Controller를 구분하신 것이므로 subway 혹은 lines로 부모 엔드 포인트를 통일하면 어떨까요?
public void insert(Line line) { | ||
List<SectionRow> sectionRows = line.getSections().stream() | ||
.map(section -> new SectionRow(null, line.getId(), section.getUpBound().getId(), | ||
section.getDownBound().getId(), section.getDistance().value())) | ||
.collect(Collectors.toList()); | ||
|
||
sectionDao.removeSections(line.getId()); | ||
sectionDao.insertAll(sectionRows); | ||
} |
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을 삽입하는 과정에서 sectionDao.removeSections()
가 필요한 이유가 무엇인가요?? 사용하는 쪽에서는 단순히 Line을 삽입한다고 생각할텐데 중간에 삭제 과정은 예상하기 어려울 것 같습니다 🤔
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.
좋은 의견 감사합니다!
현재 DIP를 적용했기에 LineRepository를 사용하는 쪽이라면 영속성에 대해 어떠한 가정을 하지 않고 있을텐데요,
호출하는 쪽에서는 단순히 Line에 대한 컬렉션으로 바라보기 때문에 내부적으로 section이 remove되는 것은 큰 문제가 아니라고 생각했었습니다.
즉, LineRepository를 사용하는 입장에서는 블랙박스처럼 line이 잘 삽입되기만 하면 된다
는 생각인데 제이온의 의견도 궁금합니다!
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.
데이터 삭제와 같은 민감한 로직은 블랙 박스로 남겨두면 추후 다른 로직으로 인해 복잡해졌을 때 큰 문제를 야기할 수 있으므로 가능한 분리를 하는 것을 권장드립니다. 가령 동료 개발자가 insert 안에 삭제 로직이 있는지 모르고 오용한다던가 할 수가 있습니다 😥
의존관계가 복잡한 경우 mock 사용
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.
안녕하세요 테오!
1차 피드백을 잘 반영해주셨고 적극적으로 자신의 의견을 작성해주신 것도 좋았습니다 👍
질문은 모두 답변 드렸고 추가적으로 생각해볼 만한 내용을 피드백으로 작성해 보았습니다.
미션은 여기서 머지해도 될 것 같고, 테코톡 준비하신 후 시간이 남는다면 3단계도 한 번 트라이해보시면 좋겠습니다 ㅎㅎ 고생하셨어요!
@@ -1,6 +1,6 @@ | |||
package subway.application.repository; | |||
package subway.application.port; |
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.
개인적으로 depth가 깊을수록 클래스 네비게이션이 번거롭기때문에 가능한 적은 depth에서 시작하여 꼭 필요한 것이 아니면 패키지를 잘 두지는 않는 편입니다. 또한 도메인 패키지 안에 model, repository, graph(혹은 좀더 추상화한 네이밍)를 두고, service 패키지는 별개로 두는 것이기 때문에 구조를 알아내기 괜찮지 않을까요??
다만 어디까지나 패키지는 개인 혹은 팀의 성향이 강하기 때문에 지금 유지하셔도 좋고 구조 변경은 선택에 맡기겠습니다 ㅎㅎ
public class DeleteStationCommand extends SelfValidating<DeleteStationCommand> { | ||
|
||
@NotNull | ||
private final Long lineId; | ||
@NotNull | ||
private final Long stationId; |
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.
말씀하셨듯이 객체 관리 비용과 매핑 비용이 생각보다 크다보니 처음에는 이들을 분리하지 않는 편입니다. 또한 무지성으로 작성하면 presentation의 DTO를 거의 application의 DTO와 동일하게 복붙하여 별 의미없이 사용하게 될 수도 있습니다.
그래서 저 같은 경우는 처음에는 DTO를 일원화하다가 presentation과 application이 서로 복잡해져서 분리할 필요성이 생기면 그때 선택적으로 해 봅니다!
물론 지금은 연습 과정이므로 극한으로 분리해보는 것도 좋은 연습이 됩니다 ㅎㅎ
public abstract class SelfValidating<T> { | ||
|
||
private Validator validator; | ||
|
||
public SelfValidating() { | ||
ValidatorFactory factory = Validation.buildDefaultValidatorFactory(); | ||
validator = factory.getValidator(); | ||
} | ||
|
||
protected void validateSelf() { | ||
Set<ConstraintViolation<T>> violations = validator.validate((T) this); | ||
if (!violations.isEmpty()) { | ||
throw new ConstraintViolationException(violations); | ||
} | ||
} | ||
} |
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.
아하 보통 @Valid
만 가지고 프레젠테이션에서 DTO를 검증하다보니 제가 실수한 부분이 있었네요!
테오께서 스스로 잘 고쳐주셨네요 ㅎㅎ 훌륭합니다.
public void insert(Line line) { | ||
List<SectionRow> sectionRows = line.getSections().stream() | ||
.map(section -> new SectionRow(null, line.getId(), section.getUpBound().getId(), | ||
section.getDownBound().getId(), section.getDistance().value())) | ||
.collect(Collectors.toList()); | ||
|
||
sectionDao.removeSections(line.getId()); | ||
sectionDao.insertAll(sectionRows); | ||
} |
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.
데이터 삭제와 같은 민감한 로직은 블랙 박스로 남겨두면 추후 다른 로직으로 인해 복잡해졌을 때 큰 문제를 야기할 수 있으므로 가능한 분리를 하는 것을 권장드립니다. 가령 동료 개발자가 insert 안에 삭제 로직이 있는지 모르고 오용한다던가 할 수가 있습니다 😥
import subway.presentation.dto.StationEnrollRequest; | ||
import subway.presentation.dto.StationResponse; | ||
|
||
import java.net.URI; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
|
||
@RestController | ||
@RequestMapping("/subway") |
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.
저는 Controller가 다르다고 path를 꼭 구분할 필요는 없다고 생각합니다~
Line이나 LineProperty나 세부 역할을 구분하기 위해 Controller를 구분하신 것이므로 subway 혹은 lines로 부모 엔드 포인트를 통일하면 어떨까요?
@Transactional | ||
@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) | ||
@Sql("classpath:dummy.sql") | ||
class JourneyServiceTest { | ||
|
||
@Autowired | ||
private JourneyService journeyService; | ||
@Autowired | ||
private LineService lineService; | ||
|
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의 동작을 보면서 테스트할 수 있다는 장점이 있습니다.
하지만 실제 DB를 이용하는 만큼 서비스의 동작을 확인하기 위해 불필요하게 DB의 준비 동작이나 후 처리 동작이 복잡해질 수 있습니다. 결국 서비스를 테스트하는 것이 아닌 Repository의 코드가 상당수 끼어들어가게 됩니다.
이때에는 Repository를 통으로 모킹해버리고 서비스의 행위만 검증하는 방식을 많이 사용합니다!
@WebMvcTest(LineController.class) | ||
class LineControllerTest { | ||
|
||
@Autowired | ||
private ObjectMapper objectMapper; | ||
@Autowired | ||
private MockMvc mockMvc; | ||
@MockBean | ||
private LineService lineService; |
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.
컨트롤러 슬라이스 테스트 👍
logger.warn(e.getMessage(), e); | ||
|
||
return ResponseEntity.badRequest() | ||
.build(); | ||
.body(new ExceptionResponse("유효하지 않은 입력입니다.")); | ||
} |
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({DataAccessException.class, SQLException.class})
public ResponseEntity<ExceptionResponse> dataException(DataAccessException e) {
logger.warn(e.getMessage(), e);
return ResponseEntity.badRequest()
.body(new ExceptionResponse("유효하지 않은 입력입니다."));
}
제가 위 코드 놓친 부분이 있는데, 모든 SQLException을 400번으로 잡아도 괜찮을까요?
만약 데이터베이스 내부 결함으로 인한 원인이 있을 수 있지 않을지 고민해 보시면 좋겠습니다~
@Repository | ||
public class LinePersistenceAdapter implements LineRepository { |
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.
Adapter도 나쁘지 않은 네이밍으로 보여지네요~
graph 구현체 네이밍 지으실때 라이브러리 이름을 적었듯이, Repository 네이밍도 JdbcTemplateLineRepository와 같이 짓는 편인데 이건 어떻게 생각하시나요?
package subway.application.core.exception; | ||
|
||
public class CircularRouteException extends ExpectedException { |
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.
시간이 되신다면 전에 언급주신 것처럼 Exception도 �어느 정도 계층화를 시도해보면 좋겠습니다~
DatabaseException, NetworkException을 두는 방법도 있고 각 계층마다 exception 패키지를 따로 두는 방법도 있습니다. 연습할 때는 전자를 추천드립니다.
안녕하세요 제이온 잘 지내셨나요 😄
2단계에서 외부 라이브러리에 대한 의존성이 추가되어서
내부/외부적 관점을 명확히 하기 위해 아키텍처 구조도 조금 변경했습니다.
이번 리뷰도 잘 부탁드립니다. 감사합니다!!
아키텍처 개요