부스트캠프 6기 iOS 코드 리뷰어 지원 🍎 #204
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 데이터 바인딩
There was a problem hiding this comment.
고생 많으셨습니다! 정말 열심히 하고 계시고 또 잘하고 계십니다! 저도 덕분에 많이 배워가고 있어요! 💪
제가 남긴 커멘트에 대해서 궁금한 점이 있다면 언제든지 편하게 질문해주세요 🙂
그리고 제 리뷰가 너무 과하다던가, 과하지 않다던가, 이런 부분에 대해서 특별히 더 살펴봐주었으면 좋겠다, 등등 피드백도 남겨주시면 참고하여 리뷰를 진행하겠습니다! 🤝
제가 남긴 피드백은 어디까지나 피드백이고, 반영 여부는 캠퍼님의 선택입니다! 저도 아직 배워가고 있는 단계이기 때문에 틀릴 수도 있어요. 중립적인 태도로 피드백을 받아들여주셨으면 좋겠고, 이 기회를 통해 코드에 관해서 함께 얘기를 나눠볼 수 있으면 좋을 것 같아요. 🙂
정말 수고 많으셨어요. 남은 프로젝트 기간동안도 화이팅입니다! 🔥🔥🔥
| return UISceneConfiguration(name: "Default Configuration", sessionRole: connectingSceneSession.role) | ||
| } | ||
|
|
||
| func application(_: UIApplication, didDiscardSceneSessions _: Set<UISceneSession>) {} |
There was a problem hiding this comment.
사용하지 않는 보일러플레이트 코드는 삭제해주세요 🙂
|
|
||
| activityVM.outputs.showFilterSheetSignal | ||
| .receive(on: RunLoop.main) | ||
| .compactMap { [weak self] in |
There was a problem hiding this comment.
클로저의 인자를 축약해서 사용하셨네요! 스위프트 공부를 많이 하신 것 같습니다!
그러나 이러한 경우에는 축약을 이용하기 보다는 인자명을 추가하거나 타입 명시를 통해 코드의 가독성을 높여보는 것은 어떨까요?
저는 커스텀 타입의 경우 읽는이가 어떤 타입의 인자를 다루는지 잘 알 수 있도록 타입 명시를 하는 편입니다.
아래의 경우에는 축약을 이용하면 작성자가 아닌 제 3자가 보았을때 $0이 무엇을 의미하는지 유추하기 어려울 것 같아보입니다. 🙂
스위프트 공식 문서에 클로저 타입 추론(Inferring Type From Context) 부분을 살펴보면, 클로저에서 타입 추론은 언제든 할 수 있지만 읽는 이에게 가독성을 높일 수 있는 경우 타입 명시를 장려한다고 합니다.
"Nonetheless, you can still make the types explicit if you wish, and doing so is encouraged if it avoids ambiguity for readers of your code."
축약 표현 또한 위와 같은 맥락으로 가독성을 해치지 않는 선에서 사용하는 편이 읽기 좋은 코드 작성에 다가가는 한걸음이 될 수 있을 것 같습니다!
| currentRange: $0.current | ||
| ) | ||
| } | ||
| .flatMap { $0 } |
There was a problem hiding this comment.
Combine의 operator flatMap을 사용하셨네요!
여기서 flatMap을 사용하신 이유를 알 수 있을까요 🙂?
|
|
||
| init(navigationController: UINavigationController, factory: ActivitySceneFactory = DependencyFactory.shared) { | ||
| self.factory = factory | ||
| super.init(navigationController: navigationController) |
There was a problem hiding this comment.
스위프트에서 상속받는 클래스의 경우 super.init()을 호출하여 상위 클래스의 초기화를 진행해주어야 하죠!
이 super.init() 전과 후로 인스턴스의 초기화의 흐름이 달라지는데요, self factory 할당을 super.init() 전에 두신 이유가 무엇인가요 🙂?
| } | ||
|
|
||
| func showActivityDetailViewController() { | ||
| // TODO: detailVM 생성 실패시 처리가 필요함!!! |
There was a problem hiding this comment.
본인에게 메모를 남기셨네요! 위와 같은 주석은 가급적이면 머지하기전에 삭제해주세요!
PR을 올리기 전에 sourcetree와 같은 GUI 깃클라이언트로 해당 브랜치에서의 변경사항을 확인하면 위와 같은 주석이 함께 올라가는 것을 방지할 수 있어요! 🙂
| } | ||
| .store(in: &cancellables) | ||
|
|
||
| // locationProvider.locationSubject |
There was a problem hiding this comment.
🚨 주석 발견! 확인 부탁드립니다 🙂
| currentSlice.setupSlice(with: states) | ||
| currentSplit.runningSlices.append(currentSlice) | ||
|
|
||
| // swiftlint:disable:next line_length |
There was a problem hiding this comment.
swiftlint disable을 활용해도 되지만, 차라리 한줄에 프린트 되지 않고 여러줄로 나누는 것이 debugger에서 확인할때도 편할 것 같은데 어떻게 생각하시나요? 🙂
| .receive(on: RunLoop.main) | ||
| .throttle(for: 2, scheduler: RunLoop.main, latest: true) | ||
| .filter { [weak self] _ in self?.autoStateChangeable ?? false } | ||
| .filter { [weak self] in $0 != self?.runningStateSubject.value } |
There was a problem hiding this comment.
filter operator를 연달아 사용하셨는데 하나의 filter operator에서 확인할 수는 없는 부분일까요? 🙂
| 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.
함수를 작성하다 만 것 같아요! 이 부분 확인 부탁드려요 🙏 🙂
| # Add this line if you want to avoid checking in source code from Carthage dependencies. | ||
| # Carthage/Checkouts | ||
|
|
||
| Carthage/Build/ |
There was a problem hiding this comment.
carthage를 사용한 흔적은 없어보이는데 .gitIgnore에 추가되어 있네요!
사소해 보일 수 있지만, .gitIgnore 또한 관리가 필요하답니다 🙂
사용하지 않는 디펜던시에 대한 ignore 명령은 삭제해주세요!
(상단을 보니 gitignore을 생성해주는 웹사이트에서 긁어오신 것 같은데 가급적이면 직접 관리하시는 것을 권장드려요! 🙂)
부스트캠프 6기 iOS 코드 리뷰어 지원합니다! 💪