부스트캠프 6기 iOS 코드 리뷰어 지원 #210
Conversation
| } | ||
|
|
||
| final class ActivityCoordinator: BasicCoordinator<ActivityCoordinationResult> { | ||
| let factory: ActivitySceneFactory |
There was a problem hiding this comment.
ActivityCoordinator 클래스 내부에서만 사용되는 변수로 보여집니다.
해당 변수를 public하게 공개시킨 이유가 있는지 궁금합니다!
공개시킬 이유가 없다면 private화 시키는 것을 제안드립니다!
| showActivityDetailScene(activity: activity, detail: detail) | ||
| } | ||
|
|
||
| func showActivityViewController() { |
There was a problem hiding this comment.
해당 함수도 위 변수와 마찬가지로 클래스 내부에서만 사용되는 함수로 보여집니다!
외부에서도 사용되는 public한 함수가 아니라면 private화 시키는 것을 추천드립니다.
내부에서만 사용되도록 구현된 함수가 외부에 공개되면 어디선가 의도하지 않게 호출될 수 있기 때문입니다!
| import UIKit | ||
|
|
||
| final class ActivityDetailCoordinator: BasicCoordinator<Void> { | ||
| let factory: ActivityDetailSceneFactory & RouteDetailSceneFactory |
There was a problem hiding this comment.
여기 지역변수들도 동일한 이유로 마찬가지로 private화 제안드립니다!
| import UIKit | ||
|
|
||
| final class ActivityListCoordinator: BasicCoordinator<Void> { | ||
| let factory: ActivityListSceneFactory |
| fatalError("start() method must be implemented") | ||
| } | ||
|
|
||
| deinit { |
There was a problem hiding this comment.
선언 순서가 다른클래스들과 동일하면 더 일관성있게 보일 것 같습니다.
다른 클래스들에서는 생성자 다음 소멸자가 선언되어있는것으로 보여지니 통일하면 더 좋을 것 같네요!
| import Foundation | ||
|
|
||
| extension CLLocationCoordinate2D { | ||
| static func computeSplitCoordinate(from points: [CLLocationCoordinate2D], distance: Double) -> [CLLocationCoordinate2D] { |
There was a problem hiding this comment.
해당 함수는 사용되지 않는것으로 보여집니다!
사용하지 않는 함수라면 삭제하는 것을 권장드립니다.
There was a problem hiding this comment.
함수의 내용과 이름이 쉽게 이해되지 않는 것 같습니다.
내용을 보면 좌표들(points)을 받아서 좌표들 사이의 누적 거리가 distance이상되는 좌표들만 다시 반환하는 것으로 이해를 했습니다.
computeSplitCoordinate라는 이름이 해당 내용을 지칭하기에는 조금 광범위 한 것 같습니다.
| let initialValue: Double = 0.0 | ||
| var splitCoordinates = [CLLocationCoordinate2D]() | ||
|
|
||
| points.reduce(initialValue) { (acculmatedDistance, currentPoint) -> Double in |
There was a problem hiding this comment.
reduce를 반복문 용도로 사용하는 것으로 이해를 했습니다.
반복문 용도로 사용하려면 reduce보다는 반복문에 사용되는 친구들로 사용하면 조금더 이해하기 수월할 것 같습니다!
| } | ||
|
|
||
| deinit { | ||
| print("[Memory \(Date())] 🌙ViewModel⭐️ \(Self.self) deallocated.") |
There was a problem hiding this comment.
클래스들마다 비슷한 부류의 메모리 문구를 사용할 것이라면 아래처럼 하나의 프로토콜로 사용하는 방법도 추천드립니다!
extension NSObjectProtocol {
func deinitLog(objectName: String? = nil) {
print("\(self.className) deallocated.")
}
}| closeSignal.send() | ||
| } | ||
|
|
||
| func viewDidLoad() {} |
There was a problem hiding this comment.
내용이 없는 함수라면 삭제해도 좋을 것 같습니다!
| struct ActivityDetailConfig { | ||
| let titleDate: String | ||
| let title: String | ||
|
|
There was a problem hiding this comment.
어떤 기준으로 줄바꿈으로 변수들을 나누었는지 궁금합니다.
| cancellable = Timer.TimerPublisher(interval: 0.8, runLoop: RunLoop.main, mode: .common) | ||
| .autoconnect() | ||
| .sink { date in | ||
| self.timeIntervalSubject.send(date.timeIntervalSinceReferenceDate) |
There was a problem hiding this comment.
클로저 내부 self참조로 retain cycle이 발생하지 않을까 의심이 됩니다.
| locationManager.delegate = self | ||
| locationManager.requestWhenInUseAuthorization() | ||
| locationManager.requestAlwaysAuthorization() | ||
| locationManager.startUpdatingLocation() |
There was a problem hiding this comment.
configureLocationManager와 동시에 startUpdatingLocation()를 진행하면 최초 권한 획득전에는 문제가 없는지 궁금합니다.
최초 진입시에는 권한획득 전이라 권한 획득 팝업이 발생할 것으로 예상되는데 startUpdatingLocation()를 호출하면 에러가 발생할 것 같습니다. 권한 획득하기 전까지의 로직을 만들어야할 것 같습니다!
| func locationManager(_: CLLocationManager, didUpdateLocations locations: [CLLocation]) { | ||
| guard let location = locations.last, | ||
| location.horizontalAccuracy.sign == .plus, | ||
| // location.verticalAccuracy.sign == .plus, |
There was a problem hiding this comment.
해당 코드는 어떠한 이유로 주석처리가 되었는지 궁금합니다!
| } | ||
|
|
||
| func stop() { | ||
| pedometer.stopUpdates() |
There was a problem hiding this comment.
start시에는 isActive = true 구문이 있는것 처럼 stop시에도 대칭되도록 isActive = false 구문이 필요할 것 같습니다.
현재 상태는 한번 시작되면 isActive가 false가 되지 않는 것 같습니다!
| } | ||
|
|
||
| func startCountingSteps() { | ||
| if isActive { return } |
There was a problem hiding this comment.
함수 이름에 맞도록 조금 더 명확한 구현을 위해서는
if isActive { return }
isActive = true
는 func start() 로 가는게 좀더 명확하지 않을까 제안드립니다!
func startCountingSteps()에서는 함수 이름에 따라 스텝 카운팅만 되도록요!
안녕하세요 👋
부스트캠프 6기 iOS 코드 리뷰어로 지원하게된 유현식입니다.
이번 참여를 통해 코드를 통해 많은 얘기를 주고받으면서 서로 배우고 성장하는 시간이 되었으면 좋겠습니다.🙇♂️