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

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

Merged
merged 48 commits into from
May 14, 2020
Merged

Conversation

ordinCode
Copy link

안녕하세요!
구현을 하면서 궁금했던 부분이 있습니다.
예외처리인데요, 백엔드에서 발생한 예외메시지를 프론트쪽으로 전달하고 싶어서 방법을 찾아보았는데 잘 못 찾겠네요.
ResponseEntity 에 메시지를 담는 방법을 시도해봤는데 잘 되지 않네요.
혹시 좋은 방법이 있을까요?

감사합니다

Copy link

@Hue9010 Hue9010 left a comment

Choose a reason for hiding this comment

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

피드백 남겼습니다! 추가로 몇가지 문의 남겼는데 확인 부탁드려요!
리뷰가 늦었네요. 😢 오늘은 빠른 피드백 가능하니 중간 중간이라도 반영해서 올리시면 확인하도록 할게요! 🙏
궁금하신 사항은 코멘트나 DM으로 문의주세요!

return ResponseEntity.ok().build();
}

@DeleteMapping("/{id:\\d+}")
Copy link

Choose a reason for hiding this comment

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

Suggested change
@DeleteMapping("/{id:\\d+}")
@DeleteMapping("/{id}")

return ResponseEntity.ok().build();
}

@PostMapping("/addStation/{id}")
Copy link

Choose a reason for hiding this comment

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

Line, Station, LineStation 중 어떤 Id인지 구분이 안됩니다. 특정 노선에 지하철 역을 추가한다는 걸 알 수 있게
/lines/{id}/stations 로 표현해주시는게 좋을 것 같습니다.
또한 post 요청에 이미 생성의 의미를 가지고 있기 때문에 add를 포함시킬 필요는 없습니다.

그리고 REST API 제대로 알고 사용하기4-2. URI 설계 시 주의할 점 > 5) URI 경로에는 소문자가 적합하다. 참고해주세요!

Comment on lines 86 to 87
Long preStationId = stationService.findStationId(map.get("preStationName"));
Station inputStation = stationService.save(map.get("stationName"));
Copy link

Choose a reason for hiding this comment

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

이 부분은 피드백을 드리기 전에 먼저 문의를 드리는게 좋을 것 같네요.

이전역은 저장되어 있는 역 중에서 조회를 하고, 대상역은 신규 생성을 하는 걸로 보이는데 요구사항에는 노선에 추가할 때의 지하철 역 존재 여부에 대한 내용이 보이진 않는 걸로 보여서 이 부분은 학성님이 어떻게 처리하고자 했는지 궁금하네요.

일단 지금 고려해 볼 수 있는 부분 중 하나는 이전역이 존재하지 않는 경우, 대상역이 중복되는 경우겠네요.

Copy link

Choose a reason for hiding this comment

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

혹시 지하철 존재여부에 대해서 고민하셨다면 해볼 수 있는 방법 중 하나는 구간 추가 화면에서 이전/대상역을 지하철 역을 조회 후 저장되어 있는 역만 셀렉 박스로 제공 할 수 있을 것 같습니다.
(이전/대상역이 없을 수 있으니 없음 같은 값도 추가가 필요하겠네요.)
방법 중 하나 일 뿐 꼭 이렇게 구현할 필요는 없습니다. 그래서 먼저 어떤 방향으로 고려를 하셨는지 여쭤보는 겁니다. 😉

Copy link

Choose a reason for hiding this comment

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

+ 기존에는 request 모델을 사용한 걸로 보이는데 map으로 바꾸신 이유가 있을까요?

Copy link
Author

@ordinCode ordinCode May 11, 2020

Choose a reason for hiding this comment

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

이부분은 착오가 있었네요.
이미 등록되어있는 역을 가지고 어느 라인, 어느 역 다음에 연결할 지 서로 mapping 시켜주는 로직인데 생성까지 했던 것은 제 실수였습니다.

그래서 이 다음 리뷰와 관련해서 생성되어 있는 지하철역들을 연결지켜주는 로직으로 변경하였습니다.
또한 휴가 말씀하신대로 구간 추가 화면에서 저장되어 있는 역을 셀렉박스로 제공하는 것도 좋은 방법으로 생각되네요. 시간관계 상 리뷰요청을 보낸뒤에 한번 시도해보겠습니다.

Copy link
Author

Choose a reason for hiding this comment

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

그리고 위에 말씀하신부분이 이 부분이 맞는지는 모르겠지만 처음에는

@PostMapping("/{id}/stations")
    public ResponseEntity addStation(@PathVariable Long id,
                                     @RequestBody String preStationName,
                                     @RequestBody String stationName,
                                     @RequestBody String distance,
                                     @RequestBody String duration) {

이렇게 했었는데 인식이 안되길래 map으로 바꾸었습니다.

map은 잘 쓰지 않는 방법인가요?

Copy link

Choose a reason for hiding this comment

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

map 많이 쓴다면 많이 쓰고 아니라면 아닙니다.
map<String, Object> -> Object에 또다른 map<String, Object> -> Object에 또다른 map<String, Object> 이 들어가 있는 지옥같은 환경일 수도 있습니다. 🙀

Request에 대한 관리가 안된다는 측면에서 저는 map의 사용은 비추합니다. 그리고 차후에 배우겠지만(아마도 빠르면 즐겨찾기에서 문서자동화 부분) request모델을 사용하면 좀더 쉽게 추가 기능들을 붙일 수 있답니다.

올려주신 코드가 안되신 이유는 http를 다시 한번 생각해보면 하나의 Request에는 하나의 body밖에는 존재하지 않습니다! 그렇기 때문에 여러개의 ReqeustBody는 사용이 불가합니다.
필요한 데이터를 xxRequest라는 이름의 Data Transfer Object로 받아아 주시면 됩니다.

</div>
</div>
<script type="module" src="./js/AdminApp.js"></script>
<script type="module" src="./js/modules/AdminStation.js"></script>
Copy link

Choose a reason for hiding this comment

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

지하철 역 관리 페이지에서 저장된 지하철 역도 안보이기 추가가 안되는 것 같은데 한번 확인 부탁드립니다.
혹시 제가 세팅(?)을 잘 못 한 걸 수도 있으니 정상적으로 되면 바로 알려주세요!

Copy link
Author

Choose a reason for hiding this comment

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

제가 놓쳤던 부분이었네요. 구현하였습니다!


@PutMapping("/{id}")
public ResponseEntity updateLine(@PathVariable Long id, @RequestBody LineRequest lineRequest) {
lineService.validateTitleWhenUpdateInfo(id, lineRequest);
Copy link

Choose a reason for hiding this comment

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

컨트롤러에서 validateTitleWhenUpdateInfo 후 update하지 말고 lineService.updateLine에서 검증하는건 어떨까요?
검증과 update가 별도로 존재하면 다른 사람이 혹은 나중에 lineService.updateLine를 사용 할 때 validateTitleWhenUpdateInfo를 별도 호출하는 걸 빠트릴 수 있습니다.

Copy link

Choose a reason for hiding this comment

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

마찬가지로 Post /lines의 validateTitle도 컨트롤러에서 별도 호출이 아니라 저장 할 때 꼭 확인 해야할 로직으로 보입니다!

Copy link
Author

@ordinCode ordinCode May 11, 2020

Choose a reason for hiding this comment

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

감사합니다. 말씀하신부분은 제가 미처 생각을 하지 못했네요.
한가지 확실히 알았으면 하는 부분이 있습니다.

저는 controller 와 service 의 역할 구분을 "작업 흐름,전처리 vs 실질적인 작업 수행" 로 생각을 하였고, 그래서 controller 에서 유효성 검사를 하였습니다.
그런데 위와 같은 상황을 생각해보니 service 에서 하는게 맞는 것도 같네요.
그러면 결과적으로 대부분의 유효성 검사는 service 에서 하는게 맞다고 생각하면 될까요?
현업에서는 어떻게 하는지 궁금합니다

Copy link

Choose a reason for hiding this comment

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

눈으로만 그냥 가볍게 볼만한 자료는 안보이네요 😿
단순히 요청에 잘 못 된 값이 왔는지 정도는 앞서 말씀 드린 Map을 제가 비선호하는 이유중 하나인데 Request의 필드들에 특정 애노테이션을 붙이는 걸로도 검증을 할 수 있습니다.

https://jojoldu.tistory.com/129 에서 보면 @NotBlank(message = "이름을 작성해주세요.") 과 같은 애노테이션입니다. 일단 눈으로만 보시고 지금 이 방법은 적용하시지 말길 바랍니다. 이유는 지금 학습 단계가 스프링등 편히 쓸 수 있는 기능을 적극적으로 활용하는 단계는 아니고 스프링의 기본적인 사용에 익숙해지는 단계로 보입니다.
여러 기능을 사용하면 더 쉽고 편하게 사용 할 수는 있지만 기본적인 동작 원리를 놓칠 수 있기 때문에 아직 학습 단계에 나오지 않았고, 검증에 대한 깐깐함이 요구되는 상황도 아니라 현재는 곁가지에 해당 하는 해당 기능을 적극 사용할 필요는 없다고 보입니다.

유효성 검사는 service에서 주로 한다기보다는 앞서 말씀 드린 애노테이션으로 request에서 요청 데이터들에 대해서 검증 할 테고, 도메인에서도 생성 시 문제가 없는지 검증, 노선 이름이 중복되는지 여부에 대해서는 db를 통해야 하기 때문에 service에서 이뤄지겠네요.

return ResponseEntity.ok().build();
}

@DeleteMapping("/station/{lineId}/{stationId}")
Copy link

Choose a reason for hiding this comment

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

lines/{lineId}/stations/{stationId}가 되어야 할 것 같네요.
실제 사용 될때를 생각해보시면 쉽답니다.
/lines/station/2/3
/lines/2/stations/3

}

public static LineResponse of(Line line) {
return new LineResponse(line.getId(), line.getName(), line.getStartTime(), line.getEndTime(), line.getIntervalTime(), line.getCreatedAt(), line.getUpdatedAt(), new HashSet<>());
return new LineResponse(line.getId(), line.getTitle(), line.getStartTime(), line.getEndTime(),
line.getIntervalTime(), line.getCreatedAt(), line.getUpdatedAt(), line.getBgColor(), new HashSet<>());
}

public static List<LineResponse> listOf(List<Line> lines) {
Copy link

Choose a reason for hiding this comment

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

사용 안하는걸로 보이네요!

Comment on lines 71 to 72
LineResponse lineResponse = LineResponse.of(line);
lineResponse.setStations(stations);
Copy link

Choose a reason for hiding this comment

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

set을 사용하지 말고 LineResponse.of(line, stations)로 생성하면 어떨까요?

추가로

이펙티브 자바 3판에서 제시하는 정적 팩토리 메서드 명명 규칙입니다.

from : 매개변수를 하나 받아서 해당 타입의 인스턴스를 반환하는 형변환 메서드
ex) Date d = Date.from(instant);

of: 여러 매개변수를 받아 적합한 타입의 인스턴스를 반환하는 집계 메서드
ex) Set<Rank> faceCards = EnumSet.of(JACK, QUEEN, KING);

valueOf : from과 of의 더 자세한 버전
ex) BigInteger prime = BigInteger.valueOf(Integer.MAX_VALUE);

instance 혹은 getInstance : (매개변수를 받는다면) 매개변수로 명시한 인스턴스를 반환하지만, 같은 인스턴스임을 보장하지는 않는다.
ex) StackWalker luke = StackWalker.getInstance(options);

create 혹은 newInstance : instance 혹은 getInstance와 같지만, 매번 새로운 인스턴스를 생성해 반환함을 보장한다.
ex) Object newArray =  Array.newInstance(classObject, arrayLen);

newType : newInstance와 같으나, 생성할 클래스가 아닌 다른 클래스에 팩터리 메서드를 정의할 때 쓴다. "Type"은 팩터리 매서드가 반환할 객체의 타입이다.
ex) BufferedReader br = Files.newBufferedReader(path);

type : getType과 newType의 간결한 버전
ex) List<Complaint> litany = Collections.list(legacyLitany);

https://www.informit.com/articles/article.aspx?p=1216151 2판 내용 링크입니다.

어디까지나 제시기때문에 절대적인 사항은 아니지만 이왕이면 일관된 규칙을 가지는게 좋답니다.

Copy link
Author

Choose a reason for hiding this comment

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

오 좋은 방법인것 같습니다!

Copy link

Choose a reason for hiding this comment

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

이 규칙을 적용하면 남들도 일반적으로 사용하는 건 사실 덤이고 이름 짓는데 소비하는 피곤함을 줄일 수도 있답니다. 😋

* <p>
* When 지하철 노선의 지하철역 목록 조회 요청을 한다.
* Then 지하철역 목록을 응답 받는다.
* And 제외한 지하철역이 목록에 존재하지 않는다.
*/
@DisplayName("지하철 노선에서 지하철역 추가 / 제외")
@Test
void manageLineStation() {
Copy link

Choose a reason for hiding this comment

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

구분이 쉽지 않아서 한글 주석 부분은 제외하고 when, then이 어떻게 나뉘는지 주석으로 넣어주는게 좋을 것 같아요.
LineAcceptanceTest의 manageLine과 같이요!

}

@PostMapping
public ResponseEntity createLine(@RequestBody LineRequest lineRequest) {
Copy link

Choose a reason for hiding this comment

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

한번에 많이 모든 곳을 처리하려 하지 말고 한번 여기에 한해서 예외 처리를 시도해보죠.

이름이 중복되면 IllegalArgumentException이 발생하는데 try-catch로 아래 형태로 응답해보세요.

return ResponseEntity.badRequest().body("message");

ResponseEntity 에 메시지를 담는 방법을 시도해봤는데 잘 되지 않네요.

사실 위의 방법도 시도해보셨을 수도 있을 것 같은데 조금 더 구체적으로 시도해보신 방법을 아는게 좋을 것 같아요. 여차하면 실패한 방법을 적용해서 push 해주시는 것도 좋을 것 같습니다.

ordinCode added 8 commits May 11, 2020 13:19
  - LineResponse.of(line,stations) 추가
 - 지하철역 추가 기능 구현
 - 페이지 로드시 저장되어있는 stations 출력 기능 구현
 - 인수테스트 수정
 - service 를 사용하도록 수정
 - create 시 유효성검사를 service 에서 검사
 - 유효성검사를 service 에서 검사
 - of 메서드 추가
 - 구간 추가 예외 발생시 처리 추가
Copy link

@Hue9010 Hue9010 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 피드백 드린 부분이 있는데 모두 다 이번에 반영해 주실 필요는 없고 다음 미션하실 때 고려해주시면 좋을 것 같아요. 이번에 반영해 주실 부분은 아래 세가지만 확인해주세요!

  • 테스트 코드 쪽 문의
  • ResponseEntity 제너릭 타입
  • xxRequest -> xx 변환

Comment on lines 64 to 70
Line line = new Line(
lineRequest.getTitle(),
lineRequest.getStartTime(),
lineRequest.getEndTime(),
lineRequest.getIntervalTime(),
lineRequest.getBgColor()
);
Copy link

Choose a reason for hiding this comment

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

lineRequest.toLine()을 통해서 line으로 변환해 줄 수 있을 것 같네요. 😉

Copy link

Choose a reason for hiding this comment

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

앗! 리뷰중 수정해주신 거 확인 했는데 LineStation을 보니 도메인 모델에서 of로 변환을 하시는데 도메인 모델에서 변환을 하시면 나중에 dto의 변경이 생길 때마다 도메인 모델도 변경을 해줘야 합니다.

Copy link
Author

Choose a reason for hiding this comment

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

아 그렇겠네요! 감사합니다


@PostMapping("/{lineId}/stations")
public ResponseEntity addStation(@PathVariable Long lineId,
@RequestBody HashMap<String, String> map) {
Copy link

Choose a reason for hiding this comment

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

이 부분은 코멘트에 남긴 것 처럼 map이 아니라 LineStationCreateRequest로 받아보세요.

public void validateTitle(Line line) {
lineRepository.findByTitle(line.getTitle())
.ifPresent(x -> {
throw new IllegalArgumentException(ERROR_MESSAGE_NAME_OVER_LAP);
Copy link

Choose a reason for hiding this comment

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

안그래도 이 부분 따로 언급하려고 했는데 생각보다 자주 발생하는 예외 케이스인데 커스텀 Exception을 만들어보세요.
DuplicationNameException, NotFoundLineException을 만들고 두가지 모두 메시지를 받지 말고 기본 생성자에서 해당 메시지를 super로 전달해주세요.

public class NotFoundLineException extends IllegalArgumentException{
    public NotFoundLineException() {
        super("해당 아이디의 노선이 존재하지 않습니다");
    }
}

Copy link

Choose a reason for hiding this comment

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

커스텀 Exception을 만들면 장점 중 하나가 에러 발생시 서버 로그를 확인 할 일이 있다면 발생한 Exception 만으로도 어떤 문제가 발생한 건지 빠르게 파악 할 수 있답니다.
스택 트레이스에 에러 발생 위치, 메시지가 찍히긴 하지만 중복된 메시지가 존재할 수도 있고 에러 발생 위치는 해당 코드 영역을 찾아가봐야 하기 때문에 xxException을 보는 것보다는 불편합니다.
대표적인 예로 NumberFormatException이 발생한 걸 본다면 어떤 문제가 발생한건지 IllegalArgumentException 또는 RuntimeException을 보는 것보다 빠르게 파악 할 수 있죠. 그리고 필요에 따라서는 예외 처리를 구분해서 처리 해 줄수도 있고요.

Comment on lines 104 to 107
@ExceptionHandler
public ResponseEntity exceptionHandler(Exception e) {
return ResponseEntity.badRequest().body(e.getMessage());
}
Copy link

Choose a reason for hiding this comment

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

👍
이 부분은 알려드릴까 말까 고민했는데 잘 찾으셨네요!
사실 이 부분을 안 알려드려서 try-catch로 처리하실 걸 예상하고 제안드리지 않은 부분이 있는데 위의
public ResponseEntity<LineResponse> findLine(@PathVariable Long id) { 처럼 다른 곳들도 모두 ResponseEntity의 제너릭 타입을 정확히 명시해 주세요.
try-catch를 사용할 때는 성공시엔 ResponseEntity<LineResponse>가 실패 시엔 ResponseEntity<String>으로 서로 다른 두가지 타입이 리턴 될 수 있어서 언급 드리지 않았었답니다.

반환 할게 없는 경우엔 ResponseEntity<Void>를 이용하시면 됩니다. 😄

참고로 try-catch과 ExceptionHandler의 차이는 메서드 레벨, 컨트롤러 레벨에서의 예외처리라는 점이 다르답니다.
스프링부트 : REST어플리케이션에서 예외처리하기

한가지 더 지금은 Exception e를 받게 되어있는데

        try {
            something
        }catch (Exception e) {
            something
        }

위와 같은 상태로 어떤 예외가 발생할지 몰라 모든 예외를 처리하겠다는 것과 마찬가지랍니다. 지금의 상태론 500대 에러인 서버에서 문제가 발생하여 생긴 상황도 badRequest로 응답을 하게 될 겁니다. 다른 부분에서 커스텀 Exception에 대해서 언급 드렸는데 커스텀 Exception을 구현하면 해당 Exception에 맞춰 핸들링하여 알맞은 응답을 내려 줄 수 있답니다. 당장은 그러한 구조로 바꾸기 위해서는 생각보다 많은 작업이 필요할 수 있어서 패스를 하거나 추가한 커스텀 Exception에 대해서만 핸들러를 추가해주시는 방법을 추천드립니다.

둘의 차이점은 지금은 Exception 발생시 어떻게든 처리가 되지만(무조건 400이라는 문제가 있긴해도..) 특정 Exception을 만들고 핸들링하면 해당 사항에 대해서는 예외 발생부터 처리까지 학성님의 통제하에 있는 상태가 된답니다. 혹여나 놓치는 부분이 나중에 발견되면 보완하면 되는 문제죠. 😉

Copy link

Choose a reason for hiding this comment

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

의외로 커스텀 Exception을 만들거나 핸들링하는게 쉽지않은 작업이라 모든 곳에 적용하는건 추천드리지 않아요. 그게 아니여도 신경 슬 것들이 많아서 더 힘드실겁니다.

그래도 조금씩 만들어보다 보면 조금은 귀찮게... 느낄수는 있어도 어렵지는 않은 작업이란걸 알 수 있어서 조금씩만 사용해보시길 추천드려요. 사실 어려운 부분은 어떤 부분에서 어떤 Exception을 어떻게 발생시키고 이름은 무엇으로 할까가 어려울거에요. 😢

Comment on lines 48 to 50
Line persistLine = lineRepository.findById(id).orElseThrow(()->{
throw new IllegalArgumentException(ERROR_MESSAGE_NAME_OVER_LAP);
});
Copy link

Choose a reason for hiding this comment

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

Suggested change
Line persistLine = lineRepository.findById(id).orElseThrow(()->{
throw new IllegalArgumentException(ERROR_MESSAGE_NAME_OVER_LAP);
});
Line persistLine = lineRepository.findById(id)
.orElseThrow(() -> new IllegalArgumentException(ERROR_MESSAGE_NAME_OVER_LAP));

throw가 빠지고 위처럼 되어야 할 것 같네요.

import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
public class LineServiceTest {
Copy link

Choose a reason for hiding this comment

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

addLineStationAtTheFirstOfLine, addLineStationAtTheEndOfLine, addLineStationBetweenTwo 에서 에러가 발생하는 걸로 보이네요. 제쪽 세팅 문제일 수도 있으니 문제 없으시면 알려주세요.

Copy link
Author

Choose a reason for hiding this comment

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

넵 수정하였습니다!

//when
List<LineResponse> lines = getLines();
LineResponse lineEight = lines.get(0);
addStation(lineEight.getId(), null, "잠실나루");
Copy link

Choose a reason for hiding this comment

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

여기서 테스트 실패하네요. 제쪽 세팅 문제일 수도 있으니 문제 없으시면 알려주세요!

Comment on lines 86 to 94
List<StationResponse> stations = getStationsByLine(line);
return LineResponse.of(line, new LinkedHashSet<>(stations));
}

private List<StationResponse> getStationsByLine(final Line line) {
return line.getLineStationsId().stream()
.map(stationId -> stationRepository.findById(stationId).orElseThrow(IllegalArgumentException::new))
.map(StationResponse::of)
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

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

서비스에서 StationsResponse를 만들지 말고 LineResponse.of(Line line, List stations)를 주고 Response 내에서 변환해주세요.
지금은 response 모델로의 변환을 service에서 일부 담당해주고 있기 때문에 resposne 모델이 변경되면 Service의 코드도 변경이 필요해 질 가능성도 있답니다.


private Set<Station> stations;
private Set<StationResponse> stations;
Copy link

Choose a reason for hiding this comment

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

👍

@Hue9010
Copy link

Hue9010 commented May 11, 2020

제가 놓친 부분이 있는데 구간 추가가 정상적으로 되고 있는지 화면에서 한번 확인 부탁드려요! + 이전역이 없는 시작 역 추가도!

ordinCode added 7 commits May 11, 2020 22:30
 - LineStationCreateRequest 를 받도록 수정
 - 커스텀 익셉션 추가
 - 반환형 변경 List<StationResponse> -> List<Station>
 - 역 삭제시 삭제 안되던 문제 해결
Copy link

@Hue9010 Hue9010 left a comment

Choose a reason for hiding this comment

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

금방 수정 가능 하실 피드백 몇개 드렸어요. 확인부탁드려요 😉

return lineRepository.save(line);
}

public List<Line> showLines() {
return lineRepository.findAll();
public void validateTitle(Line line) {
Copy link

Choose a reason for hiding this comment

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

validateTitleWhenUpdateInfo, validateTitle은 외부에서 안쓰이는걸로 보이네요.

Copy link
Author

Choose a reason for hiding this comment

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

앗! 넵 private 로 수정하였습니다.

@@ -0,0 +1,7 @@
package wooteco.subway.admin.domain.exception;

public class DuplicationNameException extends IllegalArgumentException {
Copy link

Choose a reason for hiding this comment

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

👍

Comment on lines 4 to 6
public DuplicationNameException() {
super("중복된 이름입니다");
}
Copy link

Choose a reason for hiding this comment

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

여기서 약간 더 나간다면 어떤 값(중복되는 이름)을 입력했는지 알려줄 수도 있을 것 같아요.
예) "중복된 이름입니다. [1호선]"

}

@GetMapping("/stations")
public ResponseEntity showStations() {
return ResponseEntity.ok().body(stationRepository.findAll());
public ResponseEntity<List<Station>> showStations() {
Copy link

Choose a reason for hiding this comment

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

StationResponse로 Response Model을 나누셨으니 응답은 StationResponse로 해주세요! 😉

line.getIntervalTime(), line.getCreatedAt(), line.getUpdatedAt(), line.getBgColor(), toStationResponse(stations));
}

private static Set<StationResponse> toStationResponse(Set<Station> stations) {
Copy link

Choose a reason for hiding this comment

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

LineResponse가 StationResponse로 변환되는게 아니니 이 메서드는 StationResponse.from(Station station)이 맞을 것 같네요.

Comment on lines 48 to 51
@ExceptionHandler(value = {NotFoundLineException.class, DuplicationNameException.class})
public ResponseEntity<String> exceptionHandler(Exception e) {
return ResponseEntity.badRequest().body(e.getMessage());
}
Copy link

Choose a reason for hiding this comment

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

잘해주셨어요! 👍
실제로는 @ControllerAdvice를 많이 사용하지만 try-catch 등 방법이 중요한게 아니라 핵심은 예외처리 자체입니다.

나중에 @ControllerAdvice을 사용해본다면 생각보다 간단하다는걸 느낄거에요. 그보단 매번 경험하겠지만 예외를 처리하겠다는 그 과정이 생각보다 어렵답니다. Exception e로 모든걸 다 처리하는 방법 빼고요! 😉

Copy link
Author

Choose a reason for hiding this comment

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

아 그렇군요
@ControllerAdvice 로 바꾸어 보았는데 이렇게 사용하는지 맞는지 모르겠네요ㅎㅎㅎ

Comment on lines 19 to 20
private LocalDateTime createdAt;
private LocalDateTime updatedAt;
Copy link

Choose a reason for hiding this comment

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

한가지 생각해볼 수 있는게 createdAt, updatedAt가 꼭 필요한건 아니기 때문에 도메인 모델과 1:1로 맞춰서 포함시킬 필요가 없을 수도 있답니다.

Copy link
Author

Choose a reason for hiding this comment

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

그렇겠네요.. 한번 생각해보겠습니다!

ordinCode added 4 commits May 14, 2020 12:21
 - 접근 제어자 수정
 - 이름 중복 예외 발생시 중복된 이름이 무엇인지 출력
Copy link

@Hue9010 Hue9010 left a comment

Choose a reason for hiding this comment

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

고생하셨어요! 이번 단계는 여기서 머지할게요!
추가로 궁금하신 사항있으면 코멘트나 DM으로 문의주세요!


public class DuplicationNameException extends IllegalArgumentException {
public DuplicationNameException(String title) {
super("중복된 이름입니다 -" + title);
Copy link

Choose a reason for hiding this comment

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

💯

Copy link

Choose a reason for hiding this comment

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

프로젝트 구조, 규모 등등 상황에 따라 다르기 때문에 꼭 좋은 방법이라고 할 수는 없지만 예외 발생 시 400 응답을 할 예외들은 BadRequestException을 만들어 상속시킨 후 핸들러에서 BadRequestException을 잡아주는 전략을 가질 수도 있답니다.

import wooteco.subway.admin.domain.exception.NotFoundLineException;

@ControllerAdvice
@RestController
Copy link

Choose a reason for hiding this comment

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

잘 만드셨네요. 👍
@RestController는 필요 없답니다 😉

@Hue9010 Hue9010 merged commit 78309ba into woowacourse:ordincode May 14, 2020
kang-hyungu pushed a commit that referenced this pull request May 16, 2020
KIMSIYOUNG pushed a commit that referenced this pull request May 16, 2022
* feat: 노선 등록/목록/삭제 기능 구현

* feat: 노선 조회 기능 구현

* feat: 노선 수정 기능 구현

* feat: 중복된 노선/역 이름에 대한 예외처리

* fix: 중복된 이름에 대한 상태 코드 변경

* test: 테스트 메소드 실행 전에 Dao를 초기화하는 기능 추가

* test: 노선 기능에 대한 E2E 테스트

* refactor: jdbc 기반으로 전환

* refactor: jdbc 기반으로 전환

* refactor: jdbc 기반으로 전환

* refactor: 코드 구조 리팩토링

* Merge pull request #1 from woong7/step2

* feat: 구간 관련 로직 일부 추가

* Merge pull request #2 from woong7/step2

* fix: 테스트 오류 수정

* Merge pull request #3 from woong7/step2

* refactor: 코드 전체 리팩토링

* Merge pull request #4 from woong7/step2

* refactor: Lombok / Transaction / Bean Validation 적용

* refactor: 서비스 레이어 경량화

* refactor: 전체 리팩토링

* refactor: Sections 도메인 추가

* refactor: Lines / Stations 도메인 추가

* Merge pull request #5 from woong7/step2

* feat: 역 정렬 로직 추가

* fix: 재귀 오류 수정

* refactor: 정렬 로직 수정

* Merge pull request #6 from woong7/step2

* refactor: 피드백 반영

* Merge pull request #7 from woong7/step2

* test: Sections 테스트코드 추가

* refactor: Stations / Lines 일급 컬렉션 제거

* fix: 테스트 오류 수정

* Merge pull request #8 from woong7/step2
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