Conversation
|
/domain-test |
도메인 테스트 커버리지 리포트
Generated by CI on 2026-02-22 23:58 KST |
|
Nove Domain 테스트 커버리지를 개선해야게쑥만........... |
Naknakk
left a comment
Naknakk
left a comment
There was a problem hiding this comment.
전체 구조에 대해서는 아직 고민중입니다.. .! 고민하다가 코드리뷰가 너무 늦어질 것 같아 우선 자잘한 코드 구현 자체에 대한 리뷰 올립니다!
| public mutating func setRatingThreshold(_ threshold: NovelRatingThreshold) { | ||
| if ratingThreshold == threshold { | ||
| ratingThreshold = nil | ||
| } else { | ||
| ratingThreshold = threshold | ||
| } | ||
| } |
There was a problem hiding this comment.
set 이라는 함수명이 실제 동작과 잘 매치가 안되는 것 같습니다.
현재 설정한 ratingThreshold를 제거하기 위해서 해당 함수를 호출해야 한다는 것이 많이 어색하게 느껴집니다.
차라리 입력을 optional로 받고, 무조건 ratingThreshold = threshold로 하는게 어떨까요?
다른 곳에 있는 set 함수도 마찬가지 입니다.. !
There was a problem hiding this comment.
public mutating func setRatingThreshold(_ threshold: NovelRatingThreshold?) {
ratingThreshold = threshold
}말씀해주신 내용 반영하여 단순 상태 변경만 수행하도록 수정했습니다.
고민했던 부분은
"현재 설정된 값을 다시 선택하면 해제된다"는 규칙을 도메인에 포함해야 하는지였습니다.
결론은, 해당 동작은 비즈니스 규칙보다는 사용자 인터랙션에 가까운 정책이라고 판단했습니다.
도메인은 "별점 기준을 설정하거나 해제할 수 있다"는 상태 변화만을 책임지고,
동일 값을 재선택했을 때 해제되는 토글은 Feature 레이어에서 처리하도록 역할을 분리했습니다.
좋은 리뷰 감사합니다 👍
| public func dominantReadStatus() throws -> (status: ReadingStatus, count: Int)? { | ||
| guard let dominant = readStatusCount.max(by: { $0.value < $1.value }) else { | ||
| throw ValidationError.emptyReadStatus | ||
| } | ||
| return (dominant.key, dominant.value) | ||
| } |
There was a problem hiding this comment.
반환값이 optional인데, 함수 구현을 보니 optional 반환이 불가능해 보여서 의문이 듭니다!
또한, 등록된 작품 평가가 없어서 읽기 상태 count가 모두 0인 경우에는 에러라고 볼 상황 까지는 아니라고 생각해서, nil을 반환하는게 좋을 것 같습니다.
에러를 던지는 경우는 정상적인 진행이 아닌 경우에만 던지는 것이 코드 가독성에 좋다고 생각합니다. 읽기 상태 count가 모두 0이어서 dominant가 없는 경우는 여러 작품에서 빈번히 자연스럽게 발생할 수 있는 상황이고, 이는 nil로 표현해주는게 더 자연스럽다고 느껴지네요!
에러를 던지지 않도록 수정한다면,, 함수가 아닌 computed Value로 쓰면 사용하기 훨씬 편할 것 같습니다.
가장 dominant한 readStatus 값을 읽어온다 는 느낌이기 때문에 더 직관적으로 느껴지는 것 같아요.
++ domaint readStatus를 가져오는 로직에서 지금처럼 dictionary에 max로 접근하면 안되는 이유가, 만약 데이터가
watching: 6
watched: 6
quit: 4
와 같이 오는 경우, watching과 watched가 동률이 됩니다. 이때는 읽기 상태 순서 (watching->watched->quit) 에 따라서 domaint status는 watched가 아니라 반드시 watching이 되어야 합니다..! 이 부분까지 만족할 수 있도록 다시 작성해야할 것 같아욤!
(전에 구현할 때 디자인 쌤들한테 물어봤던 기억이 있네요)
There was a problem hiding this comment.
상당히 많은 기능 명세를 놓친 것 같네요......,,, 감사합니다 🥲🥲🥲
우선 읽기상태가 없을 때 에러를 던진 이유는,
결과가 유효하게 존재해야만 의미가 있는 값이라고 판단했기 때문입니다.
따라서, 이를 nil로 반환하기 보다는 에러를 던짐으로서 읽기상태가 없는 상태라는 걸 명시하려고 했던 것 같아요.
근데 이 상황을 에러로 보기에는 무리가 있는 것 같네요!
수정해보겠습니다~!
There was a problem hiding this comment.
또한 dominant 값 조회 시 동일한 값을 가진 읽기 상태가 있을때 우선순위 고려를 놓쳤네요 ㅠㅠ
해당 사항도 반영해서 computed property로 수정하는 방향으로 진행해보겠습니다 !👍
There was a problem hiding this comment.
리뷰 내용 반영하여 dominantReadStatus를 개선했습니다.
240eced
- 정상적으로 발생 가능한 빈 상태(count가 모두 0인 경우)는 예외 대신
nil로 표현하도록 수정했습니다. throws를 제거하고computed property형태로 변경했습니다.- 같은 값 발생 시 우선순위(watching → watched → quit)에 따라 결정되도록 구현했습니다.
- 관련 테스트 케이스도 함께 추가했습니다.
꼼꼼한 리뷰 감사합니다~!
- 개별 clear 함수 접근제어자 private로 변경 - 함수명 수정 및 변수명 통일 - 테스트 코드 수정
Naknakk
left a comment
Naknakk
left a comment
There was a problem hiding this comment.
고생하셨습니다! 전체 구조는 지금 방향성 자체는 좋다고 생각이 드는데 가독성이 좀 떨어지는 느낌이 들어서요.. ! 근데 계속 고민해봐도 더 좋은 방법이 아직 안떠오르네요...ㅎ........ㅜ
회의 때 다시 한번 얘기 나눠보죠!!! |
2 participants
💡 Issue
💭 Summary
Novel Domain 모듈을 구현했습니다.
🔑 Key Changes
NovelDomain Entity 구조 개선
기존에는 Novel을 하나의 큰 Entity로 정의하여
검색 / 작품 상세 / 서재 등 여러 영역에서 공통으로 사용하려는 방향을 검토했습니다.
그러나 이 방식에는 다음과 같은 문제가 있었습니다.
특히 Domain 레이어는 명확한 상태와 불변성을 표현하는 영역이기 때문에,
다수의 Optional 필드는 오히려 모델의 의도를 흐리게 만들 수 있다고 판단했습니다.
따라서 다음과 같은 방향으로 수정했습니다:
(구조는 첨부 이미지 참고)
📱 Simulation
🧑🧒🧒 To Reviewer
※ Reference