[refactor] Mapper에서 NPE 요소 제거하기 + 커스텀 예외 적용하기 #56
Conversation
silver-eunjoo
requested review from
LimKangHyun,
dlsrks1021,
jiwon1217 and
sehee123
and removed request for
jiwon1217
| return userId; | ||
| } | ||
|
|
||
| private static String getUserNicknameIfExists(Quiz quiz) { |
There was a problem hiding this comment.
[L4-변경제안]
getUserIdIfExists()와 getUserNicknameIfExists()는 "entity to dto"의 역할을 가지는 mapper에서 수행하기 보다는 Quiz에 추가하는 것이 좋을 것 같습니다 !
There was a problem hiding this comment.
흐억!! 그렇네요. 알려주셔서 감사합니다 ! 이 부분 수정해서 다시 올려뒀습니다 ! :)
| nickname = this.creator.getNickname(); | ||
| } | ||
|
|
||
| return nickname; |
There was a problem hiding this comment.
[L4-변경제안]
id 같은 경우는 화면에 뿌려주는 용도가 아닌, js에서 사용할 것으로 보여서 어차피 로직 생성을 해야하지만, 닉네임은 화면에 보여주는 용도니까 null 일 경우 텍스트 처리를 해주면 좀 더 좋지 않을까? 생각이 드네요!
(null 일 경우"탈퇴한 사용자"로 처리)
There was a problem hiding this comment.
옹 .. 이해했습니다. null로 다 통일하면 일관성이 있을 거라 생각해서 다 null로 줬는데, 세희님 리뷰처럼 용도가 조금 다르다보니, nickname은 탈퇴한 사용자로 주는 게 더 낫겠네요 ! 이 부분 반영하겠습니다. 감사합니다 ! :)
| this.thumbnailUrl = thumbnailUrl; | ||
| } | ||
|
|
||
| public Long getUserIdIfExists() { |
There was a problem hiding this comment.
[L5-참고의견]
메서드명에 IfExists는 제외하고 Optional을 return 해주는 것도 하나의 방법일 것 같습니다.
There was a problem hiding this comment.
추가로, UserId 대신 CreatorId가 의미 전달에 효과적일 수 있을 것 같습니다.
There was a problem hiding this comment.
옹 ! 메서드명 둘 다 Creator로 변경했습니다 ! 또한, IfExists보다, 확실하게 값이 있을지 없을지 모른다는 느낌으로 get보다는 find가 맞을 것 같아 findCreatorId로 메서드명 변경했습니다 ! 좋은 의견 감사합니다 !! :)
| return this.creator.getId(); | ||
| } | ||
|
|
||
| public String getUserNicknameIfExists() { |
There was a problem hiding this comment.
[L5-참고의견]
위와 마찬가지로 메서드명의 IfExists는 제외해도 충분한 의미 전달이 가능할 것 같습니다.
🛰️ Issue Number
🪐 작업 내용
1. 커스텀 예외 적용
추가로 필요한 에러 코드가 있어서 노션에도 추가해뒀습니다 !
GameService,QuizService,QuestionService에 한해서 커스텀 예외로 변경해두었습니다.2.
QuizMapper에서 NPE 요소들 수정quiz.getCreator().getId(),quiz.getCreator().getNickname()을 수정했습니다.3. 기존 StringUtils 조건문 실수
📚 Reference
✅ Check List