-
Notifications
You must be signed in to change notification settings - Fork 304
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
[3단계 - 지하철 구간 관리] 리뷰 부탁드립니다. #742
base: jojiapp
Are you sure you want to change the base?
Conversation
jojiapp
commented
Feb 12, 2023
- 구간 등록 및 삭제
- LineStation -> Section으로 도메인 명 변경 및 패키지 분리
- 구간 등록 및 삭제 시, 검증 로직 리팩토링
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.
안녕하세요 지헌님! 😊
3단계 미션 잘 구현해 주셨네요! 👍
몇가지 참고할 만한 코멘트 남겼으니 확인해주시고 리뷰 재요청 해주시면 감사하겠습니다! 🙇
@ExceptionHandler(RuntimeException.class) | ||
public ResponseEntity<ErrorResponse> runtimeException(final RuntimeException e) { | ||
|
||
log.error("error: ", e); |
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.
에러 공통 처리를 잘 해주셨네요! 😊
예외종류에 따라 로깅레벨을 적절하게 분리해서 로그를 남기는 것도 좋을 거 같은데 어떻게 생각하시나요?
logging level 관련해서는 아래 내용을 참고해 주시면 감사하겠습니다.
예) log.error("Unexpected exception occurred: {}", e.getMessage(), e);
○ DEBUG : 개발단계에서부터 활성화.프로세스의처리순서/흐름을분석하는데 도움이 되는 정보 등
○ INFO : 디버깅정보외에 프로세스설정 / 기동에 관련한 정보 등
○ WARN : 오류상황은 아니지만, 추후확인이 필요한 정보 등
○ ERROR : 오류가 발생한상황 .즉시 대응이 필요할수있음
○ FATAL : 매우 심각한상황. 즉시 대응이 필요함
@@ -21,13 +21,27 @@ public class Line { | |||
private String color; | |||
|
|||
@Embedded | |||
@Builder.Default | |||
private LineStations lineStations = new LineStations(); | |||
private Sections sections; |
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.
`@Embedded 을 이용해 도메인 로직을 잘 분리해 주셨군요! 👍
@@ -0,0 +1,8 @@ | |||
package subway.line.section; | |||
|
|||
public class NotLastDownStationException extends RuntimeException { |
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.
Custom Exception 👍
line.addSection(createSection(request, line, upStation, downStation)); | ||
} | ||
|
||
private static Section createSection( |
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.
해당 함수는 SectionService 에서만 사용하는 것으로 보여지는데 static 으로 선언한 이유가 있을까요? 😊
return this; | ||
} | ||
|
||
private void validationLastStation(final Station section) { |
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.
매개변수 타입은 Station 인데 변수 명은 section 이여서 혼동이 있어 보여요! 😊
final Long downStationId | ||
) { | ||
|
||
final var params = new HashMap<>(); |
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.
Map 으로 만들어서 넘기기 보다는 DTO 로 만들어도 좋지 않을까요?
import static subway.given.GivenLineApi.*; | ||
import static subway.given.GivenStationApi.*; | ||
|
||
@DirtiesContext(classMode = DirtiesContext.ClassMode.BEFORE_EACH_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.
@DirtiesContext
를 활용해 테스트 격리를 해 주셨군요! 👍
@DirtiesContext
는 컨텍스트를 다시 로드하기 때문에 테스트 속도가 오래 걸린다는 단점이 있습니다!
이 부분은 한번 고민해 보시면 좋을 거 같습니다 😊
*/ | ||
@Test | ||
@DisplayName("구간 삭제 - 마지막 구간이 아니먄 예외가 발생한다.") | ||
void removeSectionThrow1() throws 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.
예외 시나리오에 대해 테스트를 잘해주셨네요! 👍