Skip to content

부스트캠프 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

Open
wants to merge 418 commits into
base: review
Choose a base branch
from

Conversation

elile-e
Copy link

@elile-e elile-e commented Aug 21, 2021

리뷰 사항

  1. 기본적인 컨벤션에 대한 의견들을 집중적으로 확인하였습니다.
  2. 메모리 누수, 퍼포먼스 이슈에 대한 부분들을 다음 우선순위로 리뷰를 작성하였습니다.

seoulboy and others added 30 commits December 8, 2020 20:39
(boostcamp-2020#91) feat: Location accuracy에 따른 값 전송 유무 로직 추가
(boostcamp-2020#88) Motion Type에 따라 자동으로 러닝을 일시정지 및 재시작한다
SHIVVVPP and others added 29 commits December 20, 2020 01:32
(boostcamp-2020#191) fix: 다크모드 및 라이트모드 toggle시 annotation이 바뀌는 버그 수정

extension UIView {
func notificationFeedback() {
print("\(#function), \(Date())")
Copy link
Author

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 "이번 주"
Copy link
Author

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 {
Copy link
Author

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)
Copy link
Author

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 }
Copy link
Author

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 {
Copy link
Author

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 {
Copy link
Author

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
Copy link
Author

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)
Copy link
Author

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
Copy link
Author

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 }
으로 처리해주면 좋지 않을까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants