부스트캠프 6기 iOS 코드 리뷰어 지원 #205
Conversation
There was a problem hiding this comment.
먼저 전체적으로 봤을 때 폴더를 구분 했더라면 조금 더 보기 편하겠다는 생각이 들었습니다. 코드 뿐만 아니라 어떻게 폴더 구조를 갖고 있는지 또한 중요하다고 생각합니다. 크게는 나눠놨지만 각 피쳐별로 나누고 공통부분은 Common 형식으로 각 폴더를 나눴다면 장기적으로 봤을 때 관리가 조금 더 용이할 것 같습니다 !
시간상 코드를 깊게 파고들지는 못했지만 많은 고민을 했던 흔적들이 느껴졌습니다. 그 덕분에 좋은 코드가 나온 것 같습니다. 고생 많이 하셨습니다 💯
|
|
||
| final class PedometerProvider: PedometerProvidable { | ||
| private let pedometer = CMPedometer() | ||
| private(set) var cadenceSubject = PassthroughSubject<Int, Never>() |
There was a problem hiding this comment.
저는 많이 사용해본적 없는데, private(set) 활용 너무 좋은 것 같습니다 :)
| func start() { | ||
| if CMPedometer.isStepCountingAvailable() { | ||
| startCountingSteps() | ||
| } | ||
| } |
There was a problem hiding this comment.
false 인 경우에 어떤 처리를 하는 로직이 들어가면 좋을 것 같습니다! 혹은 함수의 이름을 start 대신에 startWhenCountingAvailable 등의 명확한 코드의 흐름을 명시해주는게 조금 더 좋아보입니다.
| func startCountingSteps() { | ||
| if isActive { return } | ||
|
|
||
| isActive = true | ||
|
|
||
| pedometer.startUpdates(from: Date()) { [weak self] pedometerData, error in | ||
|
|
||
| guard | ||
| let self = self, | ||
| let cadence = pedometerData?.currentCadence, | ||
| error == nil | ||
| else { return } | ||
|
|
||
| self.cadenceSubject.send(Int(truncating: cadence) * 60) | ||
| } | ||
| } |
There was a problem hiding this comment.
로직을 분리하는 것 자체는 좋은 시도라고 생각합니다 👍🏻
개인적으로 아쉬운 부분을 말씀드리자면
첫번째로 외부에서 사용하지 않는 함수라면 private 을 붙여주는 것이 더욱 좋을 것 같습니다. 실제로 호출을 클래스 내부에서만 하는 것으로 확인을 했는데, 처음 봤을때는 혹시 외부에서 호출하는 것이 아닌가 하는 생각이 들었습니다. access control 또한 중요한 커뮤니케이션 도구라고 생각합니다!
두번째로 내부에서도 isActive인 경우 리턴을 하는데 상단에서 CMPedometer.isStepCountingAvailable() 의 분기를 처리하고 내부에서도 또 한번 분기를 처리를 하는 방식은 조금 생각해봐야할것 같습니다.
| func startTrackingActivityType() { | ||
| if isActive { return } | ||
| isActive = true |
There was a problem hiding this comment.
isActive 가 아닌 경우에 어떻게 처리할지 생각해보면 좋을것 같습니다. 또한 해당 값을 확인하고 리턴을 하는 경우 if 대신에 guard를 사용하는 것이 의미상으로 조금 더 가깝지 않을까? 라는 생각이 드네요 !
| if motion.isDeviceMotionAvailable { | ||
| motion.deviceMotionUpdateInterval = 1.0 / 100.0 | ||
| motion.showsDeviceMovementDisplay = true | ||
| motion.startDeviceMotionUpdates(using: .xMagneticNorthZVertical) | ||
|
|
||
| timer = Timer(fire: Date(), interval: 1.0 / 100.0, repeats: true, | ||
| block: { _ in | ||
| if let data = self.motion.deviceMotion { | ||
| self.attitudeArray.append(data.attitude) | ||
| self.gravityArray.append(data.gravity) | ||
| self.accelerometerArray.append(data.userAcceleration) | ||
| self.rotationArray.append(data.rotationRate) | ||
|
|
||
| if self.gravityArray.count >= 100 { | ||
| let result = self.getActivity( | ||
| attitude: self.attitudeArray, | ||
| gravity: self.gravityArray, | ||
| accelometer: self.accelerometerArray, | ||
| rotation: self.rotationArray | ||
| ) | ||
|
|
||
| self.attitudeArray.removeAll() | ||
| self.gravityArray.removeAll() | ||
| self.accelerometerArray.removeAll() | ||
| self.rotationArray.removeAll() | ||
|
|
||
| switch result { | ||
| case "walking": | ||
| self.motionTypeSubject.send(.running) | ||
| case "standing": | ||
| self.motionTypeSubject.send(.standing) | ||
| default: | ||
| self.motionTypeSubject.send(.standing) | ||
| } | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| RunLoop.current.add(timer!, forMode: RunLoop.Mode.default) | ||
| } |
There was a problem hiding this comment.
- 내부의 로직을 조금 분리했으면 더 좋았을 것 같습니다. whitespace 로 나누어 놓긴 했지만 timer 설정이 길어서 motion 과 timer 를 분리했다면 어땠을까 하는 생각이 드네요!
- 인터벌의 경우 위에서도 1/100 으로 설정을 했는데 만약 동일한 개념이라면 하나의 변수에 담아서 관리하는 것도 좋은 방식이지 않을까 하는 생각이 듭니다.
- switch result 의 경우 string으로 처리해서 휴먼에러를 일으키는 것 보다는, enum으로 관리 한다면 그런 걱정을 조금 줄일 수 있을 것같습니다! 하단 getActivity 함수에서 사용하는 것을 보니 enum 처리가 더욱 나아보입니다.
| private func sendChangedProfilePictureSignal() { | ||
| if currentProfile?.image != initialProfile?.image { | ||
| guard let image = currentProfile?.image else { return } | ||
| // TODO: - do something here to send signal |
There was a problem hiding this comment.
당연히 실수로 놓치셨겠지만 😢 relaese 단계에서는 TODO 등을 체크한 다음(Lint로도 가능한 것으로 알고 있습니다.) 정리하는 것이 맞다고 생각합니다. 혹은 정말로 TODO라면 따로 TODO List로 관리하고 해당 로직은 제외시키는 것이 좋다고 생각합니다.
| import Combine | ||
| import XCTest | ||
|
|
||
| class GoalTypeViewModelTest: XCTestCase { |
There was a problem hiding this comment.
처음에 구조를 볼 때 테스트는 한곳에 있을 것이라고 예상하고 따로 Tests 폴더가 없어서 의아했는데 ViewModel 근처에 있었군요 ! 생각해보니 ViewModel에서 가깝에 위치시키는 것도 파악하기 편하고 좋아보입니다 👍🏻 좋은 관점 하나 배우고 갑니다.
| private var elevationLabel1 = UILabel.makeNormal(text: "고도") | ||
| private var elevationLabel2 = UILabel.makeNormal(text: "상승") |
There was a problem hiding this comment.
init단계에서는 각 라벨이 뜻하는 고도와 상승을 확인할 수 있지만, 하단에서 Layout을 작성할 때는 1번이 고도인지 상승인지 알 수 없으며, 이로인한 실수가 발생할 수 있다고 생각합니다. 시간이 들겠지만 조금 더 명확한 네이밍을 작성하는것 것이 나중에 시간을 훨씬 줄여줄 것이라고 생각합니다.
| // MARK: - Private methods | ||
|
|
||
| // MARK: - Configure |
There was a problem hiding this comment.
먼저 MARK : - 를 이용해서 로직을 구분하니 보기 편해서 좋았습니다. 👍🏻
실수일 것이라고 생각하지만 사용하지 않는 마크는 없애거나, 혹은 다른 View파일에도 사용하지 않는 마크를 넣는것이 좋아 보입니다. 혹시 일일이 Mark를 작성하셨다면 Code Snippet도 한번 참고해보시길 바랍니다.
| import Combine | ||
| import UIKit | ||
|
|
||
| class SplitInfoDetailViewController: UIViewController { |
There was a problem hiding this comment.
사용된 컨트롤러들을 살펴보니 공통적으로 viewmodel 그리고 cancellables 를 기본적으로 사용한것을 확인할 수 있었습니다.
이렇게 공통된 부분은 Generic을 통해서 하나의 BaseViewController를 만들어 이를 상속받는 것도 하나의 방법이 될 수 있을 것 같습니다. 만약 이렇게 공통된 부분에서 하나만 변경이 되었을때 모든 파일을 다 변경하려고 한다면 굉장히 고생스러울 것으로 보입니다 😿
부스트캠프 2기때 열심히 했던 기억이 나네요 🧑🏻💻
당시에 멘토님께 리뷰를 받고 많이 배웠는데, 이번에는 제가 리뷰어가 되어 조금이나마 도움을 드릴 수 있었으면 좋겠습니다 : )