[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에서 처리하는게 낫다는 결론을 내렸습니다.
There was a problem hiding this comment.
결과값을 반환하여 ViewModel에서 사용하는 방식은 좋은 것 같습니다. 하지만 UseCase로 해당값을 전달하는 목적이라면 ViewModel의 역할과 충돌하는 역할이 될 것 같습니다.
tap(Date) -> BPM에서 반환되는 BPM을 1)화면에서 실시간으로 업데이트 하는데 사용되고, 2)다른 UseCase로 보내야 하는 역할을 동시에 수행해야 하기 때문에 차라이 이 경우에 Combine을 사용하는게 어떨까 싶습니다.
|
기절하는바람에 댓글이 늦은점 죄숭하구요 |
|
추가적으로 TapTapUseCase 관련 내용은 아니지만 전반적으로 테스트코드와 리팩토링 관련 논의가 진행되는 사항이라 한가지 더 제안해봅니다. 기존에 데이터 Sync를 맞추기 위해서 데이터 변경 후에 Repository -> UseCase로 데이터를 보내주고 있는데 이런 경우 해당 함수의 테스트 목적성에 대한 의문이 들었습니다. |
함수의 반환값을 받는 형식으로 변경하면 데이터가 전달되는 시점을 조절할 수 없게 될 것 같습니다. |
개요
TapTapUseCase를 테스트하기 좋은 코드로 리팩토링하는 목적입니다.
암튼 목적은 그럼
작업 내용
1. 책임분리
2. ViewModel 수정
3. tap() 메서드 변경
4. ReflectTempoInterface 삭제
비고
작업 스크린샷
리뷰어에게 남길 말
한번 해봤습니다. 별로일수도 있을거같아요.
CheckList