[Refactor] #376 TapTapUseCase 리팩토링 #377
Conversation
| lastTappedDate = .now | ||
| self.timeStampList.append(lastTappedDate) | ||
| lastTappedDate = timeStamp | ||
| self.timeStampList.append(timeStamp) |
There was a problem hiding this comment.
lastTappedDate가 Optional이여서 timeStamp로 직접 처리해주셨군요...
lastTappedDate말고 timeStampList자체를 Publisher로 쓰면 안될까요?
There was a problem hiding this comment.
전혀 생각해보지 않은 접근이라서 잠깐 생각해봤는데요
- timeStampList 배열을 Publisher 로 사용하면 배열을 비울때도 Publish 되는 문제가 생긴다.
- Published 프로퍼티래퍼를 사용하지 않으면 해결되나? -> 그럼 매번 배열 전체를 Publish하는건 낭비같다
- lastTappedDate를 왜 옵셔널로 바꿨더라? -> .now로 초기화되는게 마음에 걸렸다
- 근데 잠깐 lastTappedDate 시점을 저장할 필요가 있나? 어차피 Debounce로 6초 후 이벤트발생되는 구조인데?
의 연쇄적인 의문이 들었구요
lastTappedDate에 시점(Date) 정보를 저장할 필요 없이 Void 값을 반환하는 Publisher로 변경해도 아무 문제가 없을거같다는 생각이 들었습니다.
| if action != .estimateBpm { | ||
| self.taptapUseCase.finishTapping() | ||
| } | ||
| self.taptapUseCase.finishTapping() |
There was a problem hiding this comment.
해당 코드가 작동하고 있나요? 어떤 상황에서 작동하나요?
MetronomeView에서 View전체에 onTapGesture가 실행중인데 이 외 작동할 부분이 있나요?
.onTapGesture {
self.viewModel.effect(action: .disableEstimateBpm)
}
There was a problem hiding this comment.
이건 타이니와 같이 구현할때 생겼던 문제가 원인인데요
MetronomeView의 onTapGesture는 배경부분을 탭할때만 처리됩니다.
실제로 다른 액션이 발생할때는 해당 액션이 우선적으로 처리되고 MetronomeView의 VStack에 적용된 onTapGesture가 후순위인것 같더라구요
다른 액션은 Accent 변경이나 소박보기 on/off 같은 액션들이 있었습니다.
따라서 MetronomeViewModel의 effect 부분에서 빠르기찾기 기능이 아닌 다른 기능이 실행될때 Tapping을 끝내는 로직이 필요했고,
각 액션마다 코드를 추가하는것보단 상단에서 한번에 처리하는게 좋을것같아 해당 위치에 코드를 삽입하게 되었습니다.
| @@ -99,7 +99,8 @@ extension MetronomeControlViewModel { | |||
| case let .roundBpm(currentBpm): | |||
| self.tempoUseCase.updateTempo(newBpm: currentBpm) | |||
| case .estimateBpm: | |||
| self.taptapUseCase.tap() | |||
| guard let bpm = self.taptapUseCase.tap() else { break } | |||
| self.tempoUseCase.updateTempo(newBpm: bpm) | |||
There was a problem hiding this comment.
사실 TapTapUseCase도 TempoUseCase랑 통합되어야 했던건 아닐까요....?
사용자 행위를 기반으로 UseCase에서 계산된 데이터 다른 UseCase에 전달하기 위해서 ViewModel이 사용하는게 적절한건지 잘 모르겠어요....
There was a problem hiding this comment.
TapTapUseCase 에서 Repository로 BPM을 보내는 방식도 생각해봤는데 여전히 tap() 메서드에 사이드이펙트가 발생한다는 문제가 있었습니다.
각 UseCase의 명확한 기능분리를 위해 TapTapUseCase는 bpm을 '계산만' 해주고
TempoUseCase는 bpm을 Repository에 반영하는 기능만 책임지는게 좀더 낫다고 생각했습니다.
TapTapUseCase의 bpm 계산을 TempoUseCase에서 담당하게 되면
bpm에 관한 모든걸 관장하는 UseCase가 되는데 책임이 좀 많아진다는 생각이 들었고
첫줄과 마찬가지로 tap() 메서드가 bpm을 Repository에 반영까지 해준다는걸 class 외부에선 예상하기 어려울것 같습니다
변경의 목적 - 테스트하기 용이하게 - 을 고려할 때
tap() 메서드를 입력, 출력이 명확하게 변경하는게 좋겠다는 생각이 들었고
tap(Date) -> BPM 형식의 메서드라면 출력된 BPM을 어딘가에서 TempoUseCase 또는 Repository로 전달해야 합니다.
- TapTapUseCase와 TempoUseCase를 모두 참조하는 상위 UseCase를 새로 만든다
- ViewModel에서 bpm을 받아 전달한다
두가지 방안이 떠올랐고, 상위 UseCase를 만드는것보단 ViewModel에서 처리하는게 낫다는 결론을 내렸습니다.
|
기절하는바람에 댓글이 늦은점 죄숭하구요 |
개요
TapTapUseCase를 테스트하기 좋은 코드로 리팩토링하는 목적입니다.
암튼 목적은 그럼
작업 내용
1. 책임분리
2. ViewModel 수정
3. tap() 메서드 변경
4. ReflectTempoInterface 삭제
비고
작업 스크린샷
리뷰어에게 남길 말
한번 해봤습니다. 별로일수도 있을거같아요.
CheckList