feat: 근처의 명함 뷰 UI (#374) #378
Conversation
There was a problem hiding this comment.
오랜만에 어엿하게.. 성장하신 선생님의 코드를 보니 정말 눈이 즐겁습니다..!🙇🏻♂️
오랜만의 코드리뷰를 진행하니 코드가 많이 달라진듯해서 오히려 재밌고 좋습니다 ㅎㅎㅎ (진짜 ㅋㅋㅋ ㅋㅋ 엌ㅋ)
기존의 나다 프로젝트에서 좀 더 유틸적인 면에서 추가된 많은 코드들이 있는듯해요! 그리고 rx 와 mvvm 에 대한 관점까지!
코리가 많이 길구 많지만 경험이나 다르게 해보니 좋았던 점이나 코드의 관점에 대해서 하나하나 배워보도록 하겠습니다!
- getClassName 과 makeVibrate 는 파일이름인데도 소문자로 시작해서 다른 파일들과의 컨벤션에 맞지 않는듯 합니당
- controllerFromStoryBoard 또한 소문자로 시작하고, 안의 func 들을 대표하는 네이밍과도 먼 것 같습니당! controllerInStoryboard도 있으니깐요 ㅎㅎ 이게 private 여서 숨겨지는게 아니라 public 이기도하구용
- 역할에 따라서 파일을 분할하냐 관리를 위해서 어떤 것에서 extension 했냐의 관점 같아요 저는 역할에 따라 파일을 분할하면 그만큼 파일 갯수가 많아질테고 비슷한 기능이어도 파일이 분할되고, 그 기능의 분할 기준에 대해서도 모호하다고 생각해서 '...+Extension' 안에서 extension 로 분리하며 기능별로 묶는 것도 좋은 관리라고 생각하는데 이준님이 생각하신 관점이나 사용해보니 이게 좋더라하는 점이 있을까요??
흡사.. 면접 질문... 하지만.. 궁금한걸요🥲 이겨내세요 선생님.. ㅋㅋㅋ ㅋ ㅋ
주석이랑 설명 디테일하게 적어주시느라 고생하셨습니당~
| @@ -5,6 +5,7 @@ disabled_rules: | |||
| - function_parameter_count | |||
| - function_body_length | |||
| - cyclomatic_complexity | |||
| - force_cast | |||
There was a problem hiding this comment.
force unwrapping 를 쓰지 않고, 체이닝이나 옵셔널 바인딩, nil 병합연산자(nil coalescing operator)를 사용하면되서 SwiftLint 를 수정하지는 않고 경고를 무시하지 않는게 예상치 못한 종료를 방지할 수 있어서 좋을 것 같아요! 어떻게 생각하시나용
There was a problem hiding this comment.
원래 이게 말입니다... 그 controllerFromStoryBoard 파일을 가져올때 force unwrapping이 된게 있어서 그냥 냅다 린트를 고쳤거든요? 근데 그걸 이제 guard let 을 사용해서 안전하게 가져오면 이 린트파일. 다시. 원상태로. 복구하도록 하겠습니다.
There was a problem hiding this comment.
enum 형에 extension 으로 함수를 추가해서 좀 더 간단한 문법으로 접근할 수 있는거 같아요!
걱정할 사항은 기존에 Const 를 extension 해서 Storyboard 와 ViewController 구조체를 사용하고 있는데 관리할 사항이 늘어나는 것 같긴합니다.
case 순서를 보니 Storyboard 구조체의 순서랑 동일해서 인지한 상태에서 추가 구현하신거 같은데 혹시 더 나은 장점이 있으면 소개해주세용!
There was a problem hiding this comment.
마지막에 밑에 extension 지우기 전에 enum형으로 관리한걸로 모든 파일에서 바꾸고 싹 지우도록 하겠씁니당!
| extension NSObject { | ||
|
|
||
| static var className: String { | ||
| NSStringFromClass(self.classForCoder()).components(separatedBy: ".").last! | ||
| } | ||
| var className: String { | ||
| NSStringFromClass(self.classForCoder).components(separatedBy: ".").last! | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 좋은거 같아요!
현재 Const 를 extension 해서 ViewController 를 사용하는데 더이상 이부분을 관리하지 않아도 될거 같아요. UIStoryboard 를 extension 한 Storyboard enum 만 관리하면 되는게 장점이 될거 같은데 맞나요?
There was a problem hiding this comment.
이거 언래핑 변경하고 린트파일 다시 복구하도록 할게요~
| import Foundation | ||
| import UIKit | ||
|
|
||
| extension UIViewController { | ||
| public func makeVibrate(degree: UIImpactFeedbackGenerator.FeedbackStyle = .medium) { | ||
| let generator = UIImpactFeedbackGenerator(style: degree) | ||
| generator.impactOccurred() | ||
| } | ||
| } | ||
|
|
||
| extension UIView { | ||
| public func makeVibrate(degree: UIImpactFeedbackGenerator.FeedbackStyle = .medium) { | ||
| let generator = UIImpactFeedbackGenerator(style: degree) | ||
| generator.impactOccurred() | ||
| } | ||
| } |
There was a problem hiding this comment.
public 앞에 띄어쓰기가 다른 듯합니당 정렬 부탁드려여~
UIViewController 에서만 쓰이고 UIView에서는 안쓰이는거 같은데 extension 한 이유가 궁금합니당! 제가 놓친 부분이 있을까용 🥲
There was a problem hiding this comment.
놓친부분없습니다.. 그냥 긁어왔기 때문이죠 !! 아하하 !!!
정렬 완료했습니다 :)
There was a problem hiding this comment.
class func 으로 하신 이유가 궁금해요! 혹시 다른 상황에서 상속을 통해서 재정의할 목적이 있나요?
There was a problem hiding this comment.
음 딱히 이유는 없습니다! (다시 생각해봐도 없음 당연함 그냥 가져옴)
There was a problem hiding this comment.
그러면 static dispatch 로 함수를 호출할 수 있도록 static func 을 사용하면 효율적일 것 같습니당!
| func makeAroundMeVC() -> AroundMeViewController { | ||
| let viewModel = AroundMeViewModel() | ||
| let aroundMeVC = AroundMeViewController() | ||
| aroundMeVC.viewModel = viewModel | ||
| aroundMeVC.modalPresentationStyle = .overFullScreen | ||
| return aroundMeVC | ||
| } |
| class func controllerInStoryboard(_ storyboard: UIStoryboard) -> Self { | ||
| return controllerInStoryboard(storyboard, identifier: className) | ||
| } |
There was a problem hiding this comment.
NSObject 에서 static 변수가 아닌 일반 변수로 className 를 추가한 목적이 여기서 사용함이 맞을까용?
There was a problem hiding this comment.
controllerInStoryboard(_:identifier:) 메서드에 기본 파라미터를 사용해서 아래와 같이 사용해보는건 어떨까용? 그러면 함수를 오버로딩해서 사용하지 않아도 될거 같습니당!
class func controllerInStoryboard(_ storyboard: UIStoryboard, identifier: String = storyboard.className) -> Self {
return instantiateControllerInStoryboard(storyboard, identifier: identifier)
}| @@ -21,7 +21,7 @@ class SceneDelegate: UIResponder, UIWindowSceneDelegate { | |||
|
|
|||
| window = UIWindow(frame: windowScene.coordinateSpace.bounds) | |||
| window?.windowScene = windowScene | |||
| window?.rootViewController = UIStoryboard(name: Const.Storyboard.Name.splash, bundle: nil).instantiateViewController(withIdentifier: Const.ViewController.Identifier.splashViewController) | |||
| window?.rootViewController = ModuleFactory.shared.makeSplashVC() | |||
There was a problem hiding this comment.
🍾 화면관련해서 분리하니까 의미 전달이 확실한 듯합니당!
There was a problem hiding this comment.
저도 이번에 뷰 만들면서 어라 우리 네비바없이 그냥 코딩했잖아? 라는 충격을 살짝 받았어요 ㅋㅋㅋㅋ
이렇게 솔선수범.. 해주시다니 🙇🏻♂️
There was a problem hiding this comment.
저도 당황해가지고.. 아하하하 !!!!!!!!! 아유 아입니당
| required init?(coder: NSCoder) { | ||
| super.init(coder: coder) | ||
| } |
There was a problem hiding this comment.
저희 프로젝트가 스보랑 코베랑 섞여있다 보니깐 여기서도 setLayout 을 실행해주면 스보에서도 사용해볼 수 있을것 같아요!!
| import RxSwift | ||
| import RxRelay | ||
| import RxCocoa | ||
| import RxGesture | ||
| import SnapKit | ||
| import Then | ||
| import UIKit |
There was a problem hiding this comment.
오픈소스 라이브러리랑 구분해놓는건 어떨까요? 저는 요렇게 위에 내부 프레임워크를 둡니당
import UIKit
import RxSwift
import RxRelay
import RxCocoa
import RxGesture
import SnapKit
import Then| class AroundMeCollectionViewCell: UICollectionViewCell { | ||
|
|
||
| // MARK: - Properties | ||
|
|
||
| // MARK: - UI Components |
| private var cardNameLabel = UILabel().then { | ||
| $0.font = .textBold02 | ||
| } |
| dividerLine.snp.makeConstraints { make in | ||
| make.bottom.equalTo(cardNameLabel.snp.top).offset(-11) | ||
| make.height.equalTo(0.5) | ||
| } |
There was a problem hiding this comment.
PR 에 첨부해주신 사진보면 dividerLine 이 안보여서 확인한번 부탁드립니다!
| giveCardImageView.rx.tapGesture() | ||
| .when(.recognized) // bind시에도 이벤트가 발생하기 때문 .skip(1)으로도 처리 가능 | ||
| .withUnretained(self) | ||
| .bind { owner, _ in | ||
| owner.makeVibrate() | ||
| print("명함 주기") | ||
| }.disposed(by: self.disposeBag) |
| final class AroundMeViewController: UIViewController { | ||
|
|
||
| // MARK: - Properties | ||
|
|
||
| var viewModel: AroundMeViewModel! |
There was a problem hiding this comment.
final 키워드 좋습니당! 👍
viewModel 은 화면 이동시에 주입이 안되고 있는거 같은데 제가 놓친 부분이 있을까용?
강제 추출보다 옵셔널로 두시면 좋을 것 같습니당!
| private let emptyStackView = UIStackView().then { | ||
| $0.spacing = 9 | ||
| $0.axis = .vertical | ||
| $0.alignment = .center | ||
| } |
There was a problem hiding this comment.
emptyTitleLabel 과 emptyDesclabel 을 가지는 스택뷰는 간격이 제플린에 10으로 나와있어서 첨부해드립니당
| private lazy var aroundMeCollectionView = UICollectionView(frame: .zero, collectionViewLayout: aroundMeCollectionViewFlowLayout).then { | ||
| $0.showsHorizontalScrollIndicator = false | ||
| $0.clipsToBounds = false | ||
| // $0.backgroundColor = .blue | ||
| } |
| private func bindViewModels() { | ||
| let input = AroundMeViewModel.Input( | ||
| viewDidLoadEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }, | ||
| refreshButtonTapEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }) | ||
| // let output = self.viewModel.transform(from: input, disposeBag: self.disposeBag) |
There was a problem hiding this comment.
viewDidLoadEvent 에 viewWillAppear 이 연결된건 같습니당
There was a problem hiding this comment.
refreshButtonTapEvent 에 viewWillAppear 연결한건 해당 뷰에 들어오면 리프레시하는 동작을 연결하는 방식인가용? 이렇게도 사용할 수 있군요!
There was a problem hiding this comment.
엇 그러네요 다시 연결하도록 하겠습니다
어... 그리고 밑에 저건 복붙하다가 그냥 잘못 들어간거 같아요
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ
수정해놓도록 하겠습니다
There was a problem hiding this comment.
엌ㅋㅋㅋㅋ 근데 또 해당 뷰로 들어갔을 때 조회가 되야하니깐 vieewWillAppear 에서 해당 탭 이벤트를 동작시키는 방법도.. 말은 되네요 ㅋㅋㅋㅋ 오히려 좋을 수도?
There was a problem hiding this comment.
저도 당황해가지고.. 아하하하 !!!!!!!!! 아유 아입니당
| import RxSwift | ||
| import RxRelay | ||
| import RxCocoa | ||
| import RxGesture | ||
| import SnapKit | ||
| import Then | ||
| import UIKit |
| private var cardNameLabel = UILabel().then { | ||
| $0.font = .textBold02 | ||
| } |
| dividerLine.snp.makeConstraints { make in | ||
| make.bottom.equalTo(cardNameLabel.snp.top).offset(-11) | ||
| make.height.equalTo(0.5) | ||
| } |
| required init?(coder: NSCoder) { | ||
| super.init(coder: coder) | ||
| } |
| private let emptyStackView = UIStackView().then { | ||
| $0.spacing = 9 | ||
| $0.axis = .vertical | ||
| $0.alignment = .center | ||
| } |
| private func bindViewModels() { | ||
| let input = AroundMeViewModel.Input( | ||
| viewDidLoadEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }, | ||
| refreshButtonTapEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }) | ||
| // let output = self.viewModel.transform(from: input, disposeBag: self.disposeBag) |
There was a problem hiding this comment.
엇 그러네요 다시 연결하도록 하겠습니다
어... 그리고 밑에 저건 복붙하다가 그냥 잘못 들어간거 같아요
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ
수정해놓도록 하겠습니다
There was a problem hiding this comment.
음 딱히 이유는 없습니다! (다시 생각해봐도 없음 당연함 그냥 가져옴)
| private class func instantiateControllerInStoryboard<T: UIViewController>(_ storyboard: UIStoryboard, identifier: String) -> T { | ||
| return storyboard.instantiateViewController(withIdentifier: identifier) as! T | ||
| } |
There was a problem hiding this comment.
force casting을 피하고 안전하게 언래핑하기 위해
private class func instantiateControllerInStoryboard<T: UIViewController>(_ storyboard: UIStoryboard, identifier: String) -> T {
guard let viewcontroller = storyboard.instantiateViewController(withIdentifier: identifier) as? T else { return T() }
return viewcontroller
}
요런식으로 코드를 수정해보았습니다!
There was a problem hiding this comment.
넹 viewModelType 확인했습니당!
선생님이 제시해주신 것 처럼 뷰 모델이 disposebag을 가지고 있는게 좋은 구조는 아니라는 댓글이 있어서
side effect가 발생하는 쪽인 뷰컨이 가지고 있고, 그것을 가져다 쓰는 방향으로 수정하다 보니 여기에 들어간 것 같다고 합니다!
링크 첨부할게요!
https://stackoverflow.com/questions/47202871/who-should-keep-disposebag-in-mvvmcontroller-on-swift
There was a problem hiding this comment.
그러면 static dispatch 로 함수를 호출할 수 있도록 static func 을 사용하면 효율적일 것 같습니당!
| private class func instantiateControllerInStoryboard<T: UIViewController>(_ storyboard: UIStoryboard, identifier: String) -> T { | ||
| return storyboard.instantiateViewController(withIdentifier: identifier) as! T | ||
| } |
There was a problem hiding this comment.
관련 자료까지 첨부해주셔서 감사합니당!
저도 이제 프로젝트에 적용하는 단계이고 역시 MVVM 을 사용하는데 정형화된 패턴이 없기 때문에 상황에 따라서 또 달랐던거 같아여!
고생하셨습니당~
| final class AroundMeViewController: UIViewController { | ||
|
|
||
| // MARK: - Properties | ||
|
|
||
| var viewModel: AroundMeViewModel! |
| private func bindViewModels() { | ||
| let input = AroundMeViewModel.Input( | ||
| viewDidLoadEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }, | ||
| refreshButtonTapEvent: self.rx.methodInvoked(#selector(UIViewController.viewWillAppear)).map { _ in }) | ||
| // let output = self.viewModel.transform(from: input, disposeBag: self.disposeBag) |
There was a problem hiding this comment.
엌ㅋㅋㅋㅋ 근데 또 해당 뷰로 들어갔을 때 조회가 되야하니깐 vieewWillAppear 에서 해당 탭 이벤트를 동작시키는 방법도.. 말은 되네요 ㅋㅋㅋㅋ 오히려 좋을 수도?
🌴 PR 요약
🌱 작업한 브랜치
🌱 작업한 내용
📸 스크린샷
📮 관련 이슈