feat: 즐겨찾기 기능을 구현한다. #50
Conversation
Test Results59 tests 59 ✅ 6s ⏱️ Results for commit 3c3dec7. ♻️ This comment has been updated with latest results. |
| String expectExceptionMessage = new AllcllException(AllcllErrorCode.DUPLICATE_STAR, "컴퓨터구조").getMessage(); | ||
|
|
||
| // when, then | ||
| assertThatThrownBy(() -> starService.addStarOnSubject(subject.getId(), TOKEN)) | ||
| .isInstanceOf(AllcllException.class) | ||
| .hasMessageContaining(expectExceptionMessage); |
There was a problem hiding this comment.
expectedExceptionMessage를 문자열 하드코딩하는 것은 어떨까요? 위랑 다르네요
There was a problem hiding this comment.
위와 다르네요 가 어떤 말일까요..?
어느 위를 말하는걸까요???
어떤 말을 하는지 잘 이해가 안가는데 다시 한번 설명해주실 수 있으실까용
There was a problem hiding this comment.
바로 위에 있는 테스트 78번 라인을 보면
.hasMessageContaining(String.format(AllcllErrorCode.STAR_LIMIT_EXCEEDED.getMessage(), MAX_STAR_NUMBER));라고 되어 있어요. 하지만 이 테스트는 Exception 객체를 생성하고 거기에서 예외 메시지를 뽑아내고 있어요. 일관성이 안 맞아서 남긴 코멘트였어요.
| @@ -23,15 +23,15 @@ public class PinService { | |||
|
|
|||
| @Transactional | |||
| public void addPinOnSubject(Long subjectId, String token) { | |||
| List<Pin> userPins = pinRepository.findAllByToken(token); | |||
| Long pinCount = pinRepository.countAllByToken(token); | |||
| @@ -23,15 +23,15 @@ public class PinService { | |||
|
|
|||
| @Transactional | |||
| public void addPinOnSubject(Long subjectId, String token) { | |||
| List<Pin> userPins = pinRepository.findAllByToken(token); | |||
| Long pinCount = pinRepository.countAllByToken(token); | |||
There was a problem hiding this comment.
해당 라인을 validateCanAddPin 내부로 옮겨서 파라미터를 하나 줄이면 어떨까요?
| PIN_LIMIT_EXCEEDED("이미 %d개의 핀을 등록했습니다."), | ||
| STAR_LIMIT_EXCEEDED("이미 %d개의 즐겨찾기를 등록했습니다."), | ||
| DUPLICATE_PIN("%s은(는) 이미 핀 등록된 과목입니다."), | ||
| DUPLICATE_STAR("%s은(는) 이미 핀 등록된 과목입니다."), | ||
| PIN_SUBJECT_MISMATCH("핀에 등록된 과목이 아닙니다."), | ||
| STAR_SUBJECT_MISMATCH("즐겨찾기에 등록된 과목이 아닙니다."), |
|
|
||
| @Transactional | ||
| public void addStarOnSubject(Long subjectId, String token) { | ||
| Long starCount = starRepository.countAllByToken(token); |
There was a problem hiding this comment.
starCount도 validateCanAddStar 메서드 내부로 옮기면 어떨까요? starCount가 검증부에서만 사용하니까 validateCanAddStar 내부에 두면 어떨까요?
| } | ||
|
|
||
| @Test | ||
| @DisplayName("등록되지 않은 즐겨찾기의 삭제에 대한 예외를 검증한다.") |
There was a problem hiding this comment.
등록되지 않은 즐겨찾기를 삭제하면 예외가 발생한다.
이렇게 변경하는 건 어떤가요? 저는 위 이름이 이해하기 어려워서요. 어떤 행위를 했을 때, 어떤 결과가 나오는지 좀더 친절하게 적어두면 이해하기 쉬운 것 같아요
|
저는 처음에 Star 관련한 로직에서 token이 존재하지 않는다는 문제가 구현의 문제가 될 수 있겠다고 생각하여 예외를 추가해두었으나, 이는 충분히 로깅으로 추적가능하구, token이 옳지 않은 경우는 악의적 접근일 가능성이 더 높지 않을까 하는 주환님의 의견이 공감이 갔습니다. 그래서 반영했습니다~! 한번 확인해주세여 |
테스트 격리가 이루지지 않은 것 같아요.
@AfterEach가 관리하기 쉽겠네요 👍 이렇게 수정해 보아요. |
작업 내용
즐겨찾기 기능을 구현했습니다.
현재는 등록 기능만 구현해두었구요, 추후 계속 커밋 추가하겠습니다.
기존 핀 기능처럼 5개로 제한해두었어요. 핀 기능은 5개 제한인데 이거는 제한이 없는건 이상한 것 같아서요.
리뷰와 병렬적으로 가져가고자 우선 pr 올립니다.
고민 지점과 리뷰 포인트