[Step2] 인수 테스트 리팩터링 #184
[Step2] 인수 테스트 리팩터링 #184
Conversation
| Station downStation = stationRepository.findById(request.getDownStationId()).orElseThrow(() -> new DataIntegrityViolationException("Not Fount downStationId" + request.getDownStationId())); | ||
| Station upStation = stationRepository.findById(request.getUpStationId()).orElseThrow(() -> new DataIntegrityViolationException("Not Fount downStationId" + request.getUpStationId())); |
There was a problem hiding this comment.
동일한 로직이 두 번 중복됩니다. private 메소드로의 추출을 시도해볼만 하지 않을까요?
추출한다면 어디서부터 어디까지 추출하는 것이 좋을까요?
There was a problem hiding this comment.
stationId 가 매개변수로 받는 메소드로 추출되어야 하지 않을까요? 하지만, 고민되는 건 StationSerivce layer 에서도 할 수 있지만, 아직은 필요하지 않을 것 같아요.
조금더 LineSerivce 가 비대해지면 고려해볼만한 부분이라고 판단됩니다 👍
| return LineResponse.of(persistLine); | ||
| Station downStation = stationRepository.findById(request.getDownStationId()).orElseThrow(() -> new DataIntegrityViolationException("Not Fount downStationId" + request.getDownStationId())); | ||
| Station upStation = stationRepository.findById(request.getUpStationId()).orElseThrow(() -> new DataIntegrityViolationException("Not Fount downStationId" + request.getUpStationId())); | ||
| int distance = request.getDistance(); |
There was a problem hiding this comment.
굳이 별도의 지역변수를 두지 않아도 될 것 같고 Section 의 생성자 호출시에 바로 넘겨도 괜찮겠다는 생각입니다.
|
|
||
| Line save = lineRepository.save(line); | ||
| List<Station> stations = save.getStations(); | ||
| return LineResponse.of(save, stations); |
There was a problem hiding this comment.
파라메터로 Line 하나만 넘어가도, 결국 stations 까지 얻을 수 있겠는데요?
There was a problem hiding this comment.
그러네요; 코드 리팩토링이 잘 마무리되지 않은 부분인것같습니다 ㅎㅎ 꼼꼼히 코드봐주셔서 감사합니다.
| private String color; | ||
|
|
||
| public Line() { | ||
| @Embedded | ||
| private Sections sections = Sections.of(new ArrayList<>()); |
There was a problem hiding this comment.
이미 List 객체까지 초기화가 되어있는, 아래와 같은 스태틱 메소드를 구성하면 어떨까요?
public static Sections empty()There was a problem hiding this comment.
@Embedded 를 사용할 경우, 널 객체 패턴을 조심해서 활용하는게 좋을 것 같더라구요.
JPA 매직이 static 객체를 가만두지 않더군요. 하핫; 많이 당해봐서 ... 좀 더 자세한 내용은 아래 링크 첨부드렸습니다.
There was a problem hiding this comment.
@LenKIM 네, 널 객체 패턴을 사용하시라는 말씀은 아니고 Sections.of() 안에서 new ArrayList<>(); 를 넣어주면 되지 않을까 해서요.
There was a problem hiding this comment.
아 그렇군요😳 죄송합니다. 텍스트를 잘못이해했습니다 ㅎ
그럼에도 불구하고, 피드백 감사합니다 : )
| public List<Station> getStations() { | ||
| return this.sections.getValues().stream() | ||
| .map(section -> Stream.of(section.getUpStation(), section.getDownStation())) | ||
| .flatMap(Stream::distinct) |
There was a problem hiding this comment.
64 번째 라인을 살펴보시면, map을 통해서 Stream.of() 사용할 경우 Stream<Stream<Station>>으로 반환되서 distinct() 사용할 경우, Stream<Stream> 으로 반환될것입니다. 그러므로 distinct() 안될것입니다.
|
|
||
| public List<Station> getStations() { | ||
| return this.sections.getValues().stream() | ||
| .map(section -> Stream.of(section.getUpStation(), section.getDownStation())) |
There was a problem hiding this comment.
https://www.baeldung.com/java-merge-streams
두 스트림을 하나로 합치는 구현이 필요하다면 이런 방법을 생각해보시면 좋겠습니다.
There was a problem hiding this comment.
merge() 라는 메소드도 있군요 ㅎㅎ 링크 공유해주셔서 감사합니다.
|
코드 리뷰 해주셔서 감사합니다. 3단계 진행하겠습니다 |
안녕하세요. 리뷰어님! 2단계 리뷰 부탁드립니다.
코드를 잘 작성했는지 여부가 판단이 안되네요.
처음 고민했던 부분은 Section 을 어떤 기준으로 잡아야 하는지 고민이였습니다.
JPA 를 학습했지만, 활용할 때마다 늘 이렇게 할까, 저렇게 할까 고민하게 되네요.
또한, Line 에 여러 Station 을 등록할 경우 정렬을 어떤 기준으로 해야되나 고민입니다 : )
아마 3단계에서 그 문제를 해결해야 되는 것 같긴한데, 일단은 2단계 요구사항에 대해서 해결한 뒤 코드리뷰 요청드립니다.
좋은 주말 보내세요😆