부스트캠프 6기 iOS 코드 리뷰어 지원 PRReview #202
Conversation
(boostcamp-2020#25 boostcamp-2020#26) ActivityScene TotalView 연결 및 Date PickerView 구현
(boostcamp-2020#97) feat: running split scene 구현
(boostcamp-2020#91) feat: Location accuracy에 따른 값 전송 유무 로직 추가
(boostcamp-2020#88) Motion Type에 따라 자동으로 러닝을 일시정지 및 재시작한다
…e <--> EditProfileScene)
(boostcamp-2020#191) fix: 다크모드 및 라이트모드 toggle시 annotation이 바뀌는 버그 수정
…/179 (boostcamp-2020#136, 161, 179) 및 코드 컨벤셔닝
fix: Calorie 누적 오류 Int -> Double
(boostcamp-2020#196)fix: pausedWorkout 사운드 문제
(boostcamp-2020#198) feat: split detail 데이터 바인딩
|
|
||
| extension UIView { | ||
| func notificationFeedback() { | ||
| print("\(#function), \(Date())") |
There was a problem hiding this comment.
퍼포먼스에 영향을 끼칠 수 있기 때문에, 아래와 같이 DEBUG 일 경우에만 print 될 수 있도록 수정되면 좋을 것 같습니다.
#if DEBUG
print("\(#function), \(Date())")
#else
| switch self { | ||
| case .week: | ||
| if Date.isSameWeek(date: range.start, dateOfWeek: date) { | ||
| return "이번 주" |
There was a problem hiding this comment.
직접적으로 String을 다룰 경우, 유지보수 측면에서 좋지 않을 수 있습니다.
localization으로 다국어 부분과 함께 다루어지면 좋습니다.
저의 경우에는 아래의 라이브러리를 사용하여, 리소스를 관리하는 편입니다.
https://github.com/mac-cain13/R.swift
| func fetchActivityDetail(activityId: UUID) -> ActivityDetail? | ||
| } | ||
|
|
||
| protocol ActivityStorageServiceable { |
There was a problem hiding this comment.
protocol의 경우 class에서만 따르게 된다면, AnyObject or class등의 옵션을 사용한다면 불필요한 사용을 막을 수 있습니다.
| let result = try coreDataService.context.fetch(request) | ||
| return result.compactMap { $0.activity } | ||
| } catch { | ||
| print(error.localizedDescription) |
There was a problem hiding this comment.
단순히 error를 로그로 출력하기보단 예측할 수 있는 error를 구현하여 스트림에서 error를 방출 할 수 있도록 구현한다면 더욱 좋을 것 같습니다.
| var motionTypeSubject = PassthroughSubject<MotionType, Never>() | ||
|
|
||
| func startTrackingActivityType() { | ||
| if isActive { return } |
There was a problem hiding this comment.
해당 코드의 경우 guard else로 처리해 예외에 대해 가독성을 높이는건 어떨까요?
|
|
||
| timer = Timer(fire: Date(), interval: 1.0 / 100.0, repeats: true, | ||
| block: { _ in | ||
| if let data = self.motion.deviceMotion { |
There was a problem hiding this comment.
순환참조가 발생할 수 있습니다.
[weak self] _ in guard self = self else { return }
를 사용해 처리해주면 좋습니다.
ARC에 대해서도 함께 확인하면 좋을 것 같습니다!
| import Combine | ||
| import Foundation | ||
|
|
||
| class RunningService: RunningServiceType { |
There was a problem hiding this comment.
protocol을 따른다면 어디서부터 어디까지 해당 protocol을 따르는 함수 또는 변수인지 구분하여주면 좋습니다.
// MARK: RunningServiceType implementaion
xcode의 마크업 기능을 사용한다면 더욱 좋습니다!!
| private func configureLayout() { | ||
| let distanceVStack = UIStackView.make( | ||
| with: [distanceValueLabel, distancelabel], | ||
| axis: .vertical, alignment: .leading, distribution: .fill |
There was a problem hiding this comment.
개행을 하셨다면 다른 pram에 대해서도 개행을 진행해주시는게 어떤가요?
쭈욱 내려서 보는 것이 나중에 보더라도 좋다고 생각합니다!
| import UIKit | ||
|
|
||
| class DetailTotalView: UIView { | ||
| private var distanceValueLabel = NikeLabel(with: 80) |
There was a problem hiding this comment.
아래와 같이 다루게 되는 label의 숫자가 많다면 하위의 View로 묶어서 관리를 하면 어떤가요?
또, 패턴이 반복되는 layout이 있다면 묶어서 관리하면 더욱 좋겠죠?
| .receive(on: RunLoop.main) | ||
| .sink { [weak self] (bool: Bool) in | ||
| if bool { | ||
| self?.navItem.rightBarButtonItem?.tintColor = .label |
There was a problem hiding this comment.
많은 옵셔널 체이닝이 사용되고 있습니다.
이런 경우 상위에서
guard self = self else { return }
으로 처리해주면 좋지 않을까요?
리뷰 사항