feat : 커리큘럼 ID로 키워드들 조회 API 구현 #1472
Conversation
- 기존 코드 수정에 따라서 단위 테스트도 수정 #1461
|
|
||
| public static KeywordsResponse createResponseWithChildren(final List<Keyword> keywords) { | ||
| List<KeywordResponse> keywordsResponse = keywords.stream() | ||
| .filter(it -> it.getParent() == null) |
There was a problem hiding this comment.
Keyword에게 최상위 키워드인지 물어보는 메서드를 만들고 참조해도 좋을 것 같아요!
근데 한 편으로 궁금한 점이 파라미터로 받은 키워드 리스트가 최상위 키워드인지 확인하고 파싱하는 작업이 괜찮을 지 의문입니다. 외부에서 최상위 키워드만 들어오는 것보다 지금이 나은 지 궁금해요
There was a problem hiding this comment.
개인적으로
최상위 키워드
ㄴ 자식 키워드
이렇게 보여주기 위함은 UI 적인 요소라 DTO에서도 해도 되지 않을까라는 생각이 들어요.
| private final KeywordRepository keywordRepository; | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public KeywordsResponse findAllKeywords(final Long curriculumId) { |
There was a problem hiding this comment.
KeywordService에 넣지 않은 이유가 있을지 궁금합니다!
| private final RoadMapService roadMapService; | ||
|
|
||
| @GetMapping("/roadmaps") | ||
| public KeywordsResponse findKeywords(@RequestParam final Long curriculumId) { |
There was a problem hiding this comment.
로드맵 대격변 패치를 위하신 거였나보군요!
public RoadmapResponse findByCurriculumId(@RequestParam final Long curriculumId)Roadmap이라는 새로운 비즈니스 용어를 코드에서 사용하신다면 위와 같은 시그니처가 유지보수면에서 더 좋을 것 같다고 생각합니다
There was a problem hiding this comment.
findRoadMapKeyword 로 수정했습니다!
|
|
||
| @Test | ||
| @DisplayName("curriculumId가 주어지면 해당 커리큘럼의 키워드들을 전부 조회할 수 있다.") | ||
| void findAllKeywords() throws Exception { |
There was a problem hiding this comment.
커리큘럼은 controller에서 RequestParam으로 받고 있습니다.
이때 커리큘럼 ID가 존재하지 않으면 아예 서비스 쪽으로 들어오지 않을텐데 예외 테스트가 필요할까요??
There was a problem hiding this comment.
- API 요청 시 아무 Id를 적었을 때
- 해당 서비스를 다른 컨트롤러에서 사용하면 1번의 문제가 동일하게 발생
- 해당 서비스를 다른 서비스에서 의존할 때
위와 같은 상황에서 문제가 될 것 같아요!
final Curriculum curriculum = curriculumRepository.findById(curriculumId);만약 커리큘럼이 존재하지 않는 상황이 일어날 수 없다면 위와 같은 형태로 메서드 내에서 사용해야 하지 않을까요?!
|
|
||
| public static KeywordsResponse createResponseWithChildren(final List<Keyword> keywords) { | ||
| List<KeywordResponse> keywordsResponse = keywords.stream() | ||
| .filter(it -> it.getParent() == null) |
There was a problem hiding this comment.
이 부분을 isTopKeyword()와 같은 메서드로 분리하면 코드 가독성이 더 좋아질 것 같아요!
| public KeywordsResponse findAllKeywords(final Long curriculumId) { | ||
| final Curriculum curriculum = curriculumRepository.findById(curriculumId) | ||
| .orElseThrow(() -> new IllegalArgumentException( | ||
| "해당 커리큘럼이 존재하지 않습니다. curriculumId = " + curriculumId)); |
There was a problem hiding this comment.
프로젝트에서 커스텀 예외를 사용하고 있으니 가급적이면 커스텀 예외를 활용해주시는 게 유지보수 측면에서 좋을 것 같습니다!
|
RoadmapServiceTest를 실행하면 NPE가 뜨는데 혹시 이유를 아시나요? |
혹시 다시 한번 해주실 수 있나요? |
|
Kudos, SonarCloud Quality Gate passed!
|
📝작업 내용
커리큘럼 ID로 키워드들 조회