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

[소니] 지하철 정보 관리 - 인수 테스트 미션 제출합니다. #16

Merged
merged 43 commits into from
May 20, 2020

Conversation

sonypark
Copy link

@sonypark sonypark commented May 9, 2020

20.05.09 22:15
Summer 안녕하세요.
5단계까지 구현 완료했습니다.
리뷰 부탁드립니다.


20.05.09 18:00
이번 미션은 5단계까지 있는데 현재 4단계까지 완료하고 5단계 진행 중입니다.
5단계는 주말 내로 완료해서 추가 PR 보내도록 하겠습니다.
감사합니다:)

sonypark added 28 commits May 6, 2020 15:31
@sonypark
Copy link
Author

sonypark commented May 9, 2020

5단계까지 구현 완료했습니다.
리뷰 부탁드립니다.
감사합니다!!

- 직접적으로 == null 을 쓰는 대신 Objects.isNull() 메서드 사용
Copy link

@jihan805 jihan805 left a comment

Choose a reason for hiding this comment

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

안녕하세요, 깔끔하게 구현 정말 잘하셨네요 💯
몇가지 코멘트 남겼으니 확인해주세요 :)

return ResponseEntity.ok().body(lineService.showLines());
}

@GetMapping("/lines-response")
Copy link

@jihan805 jihan805 May 10, 2020

Choose a reason for hiding this comment

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

@GetMapping("/lines") 와 @GetMapping("/lines-response") 가 무엇을 나타내는지 path만 보고 유추하기가 어려운것 같아요 !
또한 @GetMapping("/lines") 의 경우 List 엔티티 객체를 리턴하고 있기도 하구요,
StationController도 같이 확인해주세요 !!

Copy link
Author

@sonypark sonypark May 10, 2020

Choose a reason for hiding this comment

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

앗 그러네요! @GetMapping("/lines") 에서 Line 엔티티 객체를 리턴할 필요가 없는데 제 실수 입니다!ㅠㅠ

LineResponse를 리턴하는 컨트롤러 하나만 있으면 돼서 기존 @GetMapping("/lines-response")를 삭제하고 @GetMapping("/lines")List<LineResponse>를 리턴하도록 수정했습니다 :) 지적 감사합니다!

public Line toLine() {
return new Line(name, startTime, endTime, intervalTime);
return new Line(name, startTime, endTime, intervalTime, bgColor);

Choose a reason for hiding this comment

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

받은 요청에 대한 validation도 체크하면 좋을 것 같아요~!

Copy link
Author

Choose a reason for hiding this comment

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

validation 로직을 어떻게 구현해야할지 감이 잘 안 잡히네요ㅠㅠ validation은 어떤식으로 하면 좋을까요?

Choose a reason for hiding this comment

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

가령 역을 추가한다고 가정했을때, 입력값 없이 역을 추가하려고 하면(예외) client에서의 처리뿐 아니라 서버에서도 이에 대한 처리를 해야 한다고 생각해요.
이렇듯 요청이 들어왔을때 이에 대한 validation을 체크할 수 있는 방안에 대해 고민해보고 먼저 간단하게라도(필수 입력값이 존재하는지 등)확인하는 로직을 추가해보면 어떨까요?

}

public static List<LineResponse> listOf(List<Line> lines) {
return lines.stream()
.map(it -> LineResponse.of(it))

Choose a reason for hiding this comment

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

사용하지 않는 메서드 혹은 import는 정리하면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

네 삭제했습니다:)


this.updatedAt = LocalDateTime.now();
}

public void addLineStation(LineStation lineStation) {
// TODO: 구현
if (lineStation == null) {

Choose a reason for hiding this comment

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

깔끔하게 구현 잘했네요 👍

@@ -22,33 +27,70 @@ public LineService(LineRepository lineRepository, StationRepository stationRepos
}

public Line save(Line line) {
List<Line> persistLines = lineRepository.findAll();
boolean hasDuplicateName = persistLines.stream()
.anyMatch(persistLine -> persistLine.getName().equals(line.getName()));

Choose a reason for hiding this comment

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

혹은 exists 로 중복 체크를 할 수 있을 것 같아요~!

Copy link
Author

Choose a reason for hiding this comment

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

CrudRepositoryexistById라는 메서드가 있었네요!

LineRepository에 아래 메서드를 만들어서 노선 이름 중복체크 기능을 구현했습니다.

@Query("SELECT COUNT(*) > 0 FROM LINE WHERE name = :name")
    boolean existsByName(@Param("name") String name);

Copy link

@jihan805 jihan805 left a comment

Choose a reason for hiding this comment

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

안녕하세요 피드백 반영 잘하셨어요 💯
간단한 코멘트 남겼으니 확인해주세요 :)

public Line toLine() {
return new Line(name, startTime, endTime, intervalTime);
return new Line(name, startTime, endTime, intervalTime, bgColor);

Choose a reason for hiding this comment

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

가령 역을 추가한다고 가정했을때, 입력값 없이 역을 추가하려고 하면(예외) client에서의 처리뿐 아니라 서버에서도 이에 대한 처리를 해야 한다고 생각해요.
이렇듯 요청이 들어왔을때 이에 대한 validation을 체크할 수 있는 방안에 대해 고민해보고 먼저 간단하게라도(필수 입력값이 존재하는지 등)확인하는 로직을 추가해보면 어떨까요?

this.lineService = lineService;
}

@PostMapping("/lines")

Choose a reason for hiding this comment

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

LineController의 path는 전체적으로 /lines로 시작하는데 @RequestMapping을 사용하여 클래스에 매핑해주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

네 path에서 공통된 부분은 @ReauestMapping으로 추출하여 LineController, StationController에 모두 적용했습니다. :)

@sonypark
Copy link
Author

@Valid, @RestControllerAdvice 어노테이션을 이용하여 요청이 들어왔을때 이에 대한 validation을 체크 로직을 추가했습니다.

Copy link

@jihan805 jihan805 left a comment

Choose a reason for hiding this comment

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

안녕하세요 소니, 피드백 반영 & 구현 모두 잘하셨어요 💯
머지할게요, 고생 많으셨어요 :)

@jihan805 jihan805 merged commit 211d5e0 into woowacourse:sonypark May 20, 2020
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