-
Notifications
You must be signed in to change notification settings - Fork 10
부스트캠프 6기 iOS 코드 리뷰어 지원 PRReview #202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: review
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
퍼포먼스에 영향을 끼칠 수 있기 때문에, 아래와 같이 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
직접적으로 String을 다룰 경우, 유지보수 측면에서 좋지 않을 수 있습니다.
localization으로 다국어 부분과 함께 다루어지면 좋습니다.
저의 경우에는 아래의 라이브러리를 사용하여, 리소스를 관리하는 편입니다.
https://github.com/mac-cain13/R.swift
func fetchActivityDetail(activityId: UUID) -> ActivityDetail? | ||
} | ||
|
||
protocol ActivityStorageServiceable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
단순히 error를 로그로 출력하기보단 예측할 수 있는 error를 구현하여 스트림에서 error를 방출 할 수 있도록 구현한다면 더욱 좋을 것 같습니다.
var motionTypeSubject = PassthroughSubject<MotionType, Never>() | ||
|
||
func startTrackingActivityType() { | ||
if isActive { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 코드의 경우 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
순환참조가 발생할 수 있습니다.
[weak self] _ in guard self = self else { return }
를 사용해 처리해주면 좋습니다.
ARC에 대해서도 함께 확인하면 좋을 것 같습니다!
import Combine | ||
import Foundation | ||
|
||
class RunningService: RunningServiceType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개행을 하셨다면 다른 pram에 대해서도 개행을 진행해주시는게 어떤가요?
쭈욱 내려서 보는 것이 나중에 보더라도 좋다고 생각합니다!
import UIKit | ||
|
||
class DetailTotalView: UIView { | ||
private var distanceValueLabel = NikeLabel(with: 80) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아래와 같이 다루게 되는 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
많은 옵셔널 체이닝이 사용되고 있습니다.
이런 경우 상위에서
guard self = self else { return }
으로 처리해주면 좋지 않을까요?
리뷰 사항