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 경로 조회 - 1단계] 후디(조동현) 미션 제출합니다. #203

Merged
merged 18 commits into from
May 20, 2022

Conversation

devHudi
Copy link

@devHudi devHudi commented May 18, 2022

안녕하세요 럿고! 처음 뵙네요, 후디입니다! 😄
처음으로 제 코드가 아닌 페어의 코드로 시작한 미션이네요!
아직 부족한 점이 많습니다. 잘 부탁드려요 🙇‍♂️

@devHudi
Copy link
Author

devHudi commented May 18, 2022

(지금 당장 마주친 문제는 아니지만) 서로 다른 Service 끼리의 중복되는 로직이 많이 발생하게 되면 Service 간 참조를 고려할 수도 있다고 생각합니다. 예를 들어 AService 에서 A 도메인 객체를 생성하기 위해 여러 DAO 호출이 필요해 로직이 복잡해졌고, 나중에 BService 에서도 A 도메인을 생성해야하는 상황이 발생했다고 가정해볼게요. 직관적으로 BServiceAService 를 직접 호출하여 중복 로직을 제거할 수 있을 것 같아요.

하지만, 이 방식은 Service 간의 순환 참조로 이어질 수도 있다고 생각해요. 따라서 개인적으로 저는 Layered Architecture 구조에서 의존의 방향은 단방향 (Controller -> Service -> Dao -> Database) 여야한다고 생각합니다. 하지만 그렇게 된다면 복잡한 A 도메인을 생성하기 위한 로직이 AServiceBService 둘다 중복적으로 존재하게 되는데요. 이런 상황에 대한 럿고의 의견이 궁금합니다! 🧐


@Test
@DisplayName("구간이 2개일 때 상행 종점을 제거한다.")
void deleteUpTerminal() {

Choose a reason for hiding this comment

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

여기서부터 아래까지 공통된 포맷에 대한 테스트 인거 같네요. 한번 @ParameterziedTest를 사용해보면 좋겠네요.


@Test
@DisplayName("구간을 추가하면 노선에 포함된 역이 한 개 늘어난다.")
void addSection() {

Choose a reason for hiding this comment

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

여기도 아래서부터 같은 포맷의 테스트 같은데 @ParameterziedTest를 사용해보면 어떨까요?

Copy link

@ksy90101 ksy90101 left a comment

Choose a reason for hiding this comment

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

후디 안녕하세요 :)
지하철 미션 하시느라 고생 많으셨습니다 💯
간단한 피드백을 남겼으니 확인해주시면 감사하겠습니다!
이 피드백은 충분히 다음 미션에서도 적용이 가능할거 같아서 이번 미션은 머지할게요~
질문이 있다면 언제든지 슬랙으로 연락주세요!!

Comment on lines +14 to +17
this.sections = new ArrayList<>() {
{
add(section);
}

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/1958636/what-is-double-brace-initialization-in-java
이글을 한번 읽어보시면 좋을거 같습니다.

private static final String INTERNAL_ERROR_MESSAGE = "서버에 오류가 있으니 잠시 후 다시 시도해주세요.";

@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity<?> handleIllegalArgumentException(RuntimeException e) {

Choose a reason for hiding this comment

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

Suggested change
public ResponseEntity<?> handleIllegalArgumentException(RuntimeException e) {
public ResponseEntity<String> handleIllegalArgumentException(RuntimeException e) {

아닐까요!?

}

private static int calculateOverFareWithDistanceUnit(int distance, int unitDistance) {
int numberOfImposition = (distance - 1) / unitDistance + 1;

Choose a reason for hiding this comment

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

1은 매직넘버 인거 같네요ㅎㅎㅎ

return ResponseEntity.created(URI.create("/stations/" + newStation.getId())).body(stationResponse);
}

@GetMapping(value = "/stations", produces = MediaType.APPLICATION_JSON_VALUE)
@GetMapping(produces = MediaType.APPLICATION_JSON_VALUE)

Choose a reason for hiding this comment

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

이 부분만 produces를 붙인 이유가 있을까요?

@ksy90101 ksy90101 merged commit 93b27df into woowacourse:devhudi May 20, 2022
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.

3 participants