[Team10 / iOS / 쑤 & Zeke] Rx를 활용한 ViewModel 구현 #48
Conversation
Conflicts: iOS/issue-tracker/issue-tracker.xcodeproj/project.pbxproj iOS/issue-tracker/issue-tracker/Controller/AddViewController.swift
Conflicts: iOS/issue-tracker/issue-tracker.xcodeproj/project.pbxproj iOS/issue-tracker/issue-tracker/Controller/LabelViewController.swift
[iOS] Complement UI
Conflicts: iOS/issue-tracker/issue-tracker/ViewModel/MilestoneViewModel.swift
[iOS] complement tableView cell and the other features
There was a problem hiding this comment.
- 메모리 누수 문제가 있는 코드가 2군데 있습니다. 제거 부탁드릴게요.
- lazy var 는 메모리 효율을 위해 사용하기 부담스러운 변수들에 대해서만 사용합니다. 지금 살펴보니 모두 필요한 변수들이 단순히 먼저 선언 및 생성 코드를 적기 위해 사용하는 것 같아요. 모두 lazy var 없이 처리할 수 있는 변수들입니다(심지어 지금 lazy var 5개의 변수 중 2개는 let 처리해도 빌드되네요). 모두 lazy를 없애주세요. 항상 기준을 가지고 코드를 작성해야 합니다! 😄
| private lazy var httpHeaders: HTTPHeaders = ["Content-Type": "application/json", | ||
| "Accept": "application/json", | ||
| "Authorization": getToken()] | ||
|
|
||
| func getToken() -> String { | ||
| guard let token = UserDefaults.standard.string(forKey: "token") else { | ||
| return "" | ||
| } | ||
| return token | ||
| } |
There was a problem hiding this comment.
- 두 개의 용도를 보니 모두 연산 프로러티로 만들수 있겠네요. 그럼 lazy 키워드도 필요없지 않을까요?
- getToken 은 private이 빠져있네요. 내부에서만 사용하는 거면 private 추가하면 어떨까요? 일단 지금은 내부에서만 사용하는군요.
| } | ||
| } | ||
|
|
||
| func setUpAlertView(title: String, message: String, buttonTitle: String, alertType: AlertType, buttonHandler: (() -> Void)?) { |
There was a problem hiding this comment.
사실 Util의 static func 로 싱글톤 알랏뷰 없이 알랏을 띄울 수 있습니다. 인자로 viewController를 넘기면 되겠네요.
그리고 싱글톤은 가장 쉬우면서도 문제가 많은 해결책입니다. 되도록이면 안쓰는 것을 권장합니다.
쓰신다면 구글링으로 싱글톤으로 많이 찾아보고 기준을 갖고 사용하시면 될 것 같아요.
| class AddMilestoneViewController: UIViewController { | ||
|
|
||
| private let disposeBag = DisposeBag() | ||
| private let viewModel: MilestoneViewModel! = MilestoneViewModel.shared |
There was a problem hiding this comment.
뷰모델을 싱글톤으로 사용하는 것도 안됩니다. 인스턴스 한 뷰들에 대응하는 인스턴스한 뷰모델이 좋아요.
| import RxCocoa | ||
|
|
||
| class MilestoneViewModel { | ||
| static let shared = MilestoneViewModel() |
There was a problem hiding this comment.
ViewModel의 역할
지금은 뭔가 뷰모델이 뷰모델 같지 않고 네트워크 단 객체이자 뷰나 뷰컨트롤러를 컨트롤하는 역할로만 보입니다.
- 네트워크 매니저를 갖음으로서, 네트워크 역할을 자기 메소드로 수행한다.
- 알랏 뷰를 띄워주는 역할을 한다.
- ViewController를 dismiss 하는 역할도 한다.
뭐 NetworkManager를 안에 가지고 있는 변형된 뷰모델도 많고, 괜찮다는 사람이 매우 많아서 넘어가겠습니다. 하지만 메인 포인트는 뷰모델로부터 네트워크 단을 분리해도 뷰모델 객체의 역할은 유효하다는 것입니다. 그럼 뷰모델의 역할은 무엇일까요?
MVP의 Presenter, MVVM의 ViewModel의 제일 중요 역할은 뷰에 뿌려질 데이터를 가공하는 프레젠터(Presenter)의 역할입니다. SideDish로 예를 들면, 모델에 가격데이터가 있고 뷰에 0이 3개당 점(,)을 찍어야 된다고 합시다. 그럼 점 찍는 역할을 뷰모델이 해줘야 합니다.
var model = Model()
var viewModel = ViewModel(model)
var view = viewModel.priceText
class ViewModel {
var priceText: String {
return 점찍은 돈 String 값
}
}이러면 ViewModel은 뷰와의 의존성 없이 테스트도 가능합니다. 이래서 뷰모델을 사용하는 것이죠.
지금 제가 판단하기로는 뷰모델이 프레젠터로서 역할을 하고 있는지 잘 모르겠는데 점검해보시는 게 좋을 것 같아요. 사실 뷰에 값을 바인딩할때도 값을 가공(즉 뷰에 알맞게 프레젠팅)하고 넘기면 돼요 (ex. bind(to: view))
| } | ||
|
|
||
| private func fetch(completion: (() -> Void)? = nil) { | ||
| networkManager.request(url: url, decodableType: MilestoneList.self) { [weak self] data in |
There was a problem hiding this comment.
request 메소드 인수에 httpMethod도 인수로 추가하면 더 재사용성 올라가고, 함수가 중복되지 않아서 더 좋을 것 같은데 어떡해 생각하시나요? 뭔가 이번 기수의 유행처럼 httpMethod 별로 메소드를 만드신것 같군요 🧐
There was a problem hiding this comment.
그리고 request는 사실 데이터를 요청만 하는게 아니라 decode 까지 하고 있어요. 그래서 엄밀하게 말하면 requestAndDecode가 맞아요. 따라서 역할에 맞게 분리해줘야 합니다.
- 그럼 AF의
responseDecodable은 못쓰는 걸까요?
라는 질문이 나올 것 같아요. 네 사실 AF으로 여러분이 구현하신건 OOP가 아닌 FP에요. 따라서 FP대로 잘 쓰려면 현재 코드상 networkManager없이 바로 지금 쓰이는 MilestoneViewModel에서 바로 써줘야 합니다. OOP 를 대신해 FP를 사용한 것이니까요.
There was a problem hiding this comment.
- 같은 뷰모델이더라도 DI를 지킨 부분이 있고 지키지 않은 부분이 있네요! DI의 장점 중 제일 직관적인 것은 테스트하기 좋다는 것입니다. 테스트도 해보시는 걸 추천드립니다. 그럼 왜 의존성 주입(의존성 역전 원칙 포함)을 해야 하는지 한번에 아실거에요.
- 또 뷰모델도 테스트하시는 걸 추천드려요. Presenter로서 뷰모델이 충분한 역할을 한다면 뷰에 보여질 데이터 검증을 뷰와의 의존성 없이 테스트 할 수 있습니다 👍
해결하지 못한 것
Issue List 화면에서 Issue의 Label의 개수(IssueLabelCollectionView의 크기)에 따라 Table View Cell의 높이를 동적으로 구현하는 것을 해결하지 못했습니다.
시도한 것: LabelCollectionView의 ContentSize를 이용해 TableViewCell의 높이를 지정해주려 했지만 ContentSize가 지정되는 시점이 TableViewCell의 높이를 결정하는 시점보다 나중이어서 실패
블로그를 참고해봤지만, 현재 rx를 활용하고 있어 그대로 적용하기 어려웠습니다.😥
=> 네, 제가 말씀드렷던 블로그가 해당 블로그입니다, 이건 방법이 없습니다.
- RxSwift 에 해당 이슈를 해결하는데 도움이 되는 operator를 찾거나
- rx때문에 구현하기 어렵다면 rx 코드를 버리고서라도 구현해야 합니다. 문제 해결이 테크닉보다 중요하기 때문입니다.
네, 이렇게 리뷰를 마치도록 하겠습니다. 피드백 반영 부탁드릴게요 💪
|
|
||
| private var viewModel = NewIssueViewModel() | ||
| private let disposeBag = DisposeBag() | ||
| weak var delegate: NewIssueViewDelegate? |
There was a problem hiding this comment.
정확히 말하면 NewIssueViewControllerDelegate 인거죠? 변수명도 최대한 정확한게 좋아요 ㅎㅎ
There was a problem hiding this comment.
Delegate 패턴
Issue 새로 등록 하고 나서 Issue List 데이터를 refresh 해주기 위해 delegate 패턴을 사용
로직을 따라가기 쉽고, 커뮤니케이션 과정을 모니터링하는 제 3의 객체(ex. NotificationCenter)가 필요 없는 상황이므로 delegate 패턴을 사용하였습니다.
- 네, 지금처럼 1:1 관계이므로 delegate 패턴을 사용하신게 적절해 보입니다! 아시겠지만 추후에 기획이 바뀌어(회사의 프로젝트라고 가 정) 1: N 관계로 바뀐다면 Notification 패턴을 고려하셔야 합니다~
- delegate 에 weak 선언도 잘해주셨네요! 👏👏👏
| fatalError("init(coder:) has not been implemented") | ||
| } | ||
|
|
||
| func configureTextFieldPlaceHolder(text: String) { |
There was a problem hiding this comment.
클린코드 2장: 의미있는 이름 - 한 개념에는 한 단어만 사용하라
지금 같은 개념에 setup, configure 두 가지 단어를 함께 쓰시는 것 같은데 한 단어로 통일하는게 좋아요.
왜냐하면 이 프로젝트에 다른 멤버가 추가되었을때 같은 개념에 두 단어를 함께 쓴다면 헷갈릴 여지가 있기 때문입니다. 아니면 똑같은데 왜 다르지?? 라는 생각을 할 수 있을 것 같아요.
|
|
||
| @objc func requestToken() { | ||
| authManager.requestJWTToken(completion: { | ||
| let st = UIStoryboard(name: "Main", bundle: nil) |
There was a problem hiding this comment.
저는 변수명은 어떤 스토리보드인지 어떤 뷰컨트롤러인지 명시되도록 길게 쓰는 편입니다. Swift 가이드라인에서도 최대한 표현되도록 길게 쓰라고 나와있고, 자바로 설명된 책 클린코드에서도 최대한 표현되도록 길게 쓰라고 말합니다.
왜 길게 써야 하나면 다른 동료가 해당 코드를 읽을때 왼쪽 내용을 읽기만 해도 코드 파악이 되기 때문입니다.
let mainViewController = mainStoryBoard.instantiateViewController(withIdentifier: "main")
mainViewController.modalPresentationStyle = .fullScreen그리고 신뢰도 생깁니다. 이렇게 명시되어있으면 굳이 main 스토리보드까지 안가도 동료개발자는 작업을 진행할 겁니다.
하지만 줄인말을 사용하면 해당 코드에 의심이 생겨 저라면 스토리보드까지 가서 identifier가 main 인 viewController를 확인해볼 것 같아요.
네, 확인해보니 MainViewController라는 것은 없고 UITabBarController군요. 그럼 tabBarController라는 것을 알려주는 변수명을 지으면 동료 개발자(특히 신입)가 더욱 확신이 생겨서 앱 플로우를 명확히 알 것 같아요. 변수명은 꽤 중요한 것 같습니다. 😀
let mainTabBarController = mainStoryBoard.instantiateViewController(withIdentifier: "main")
mainTabBarController.modalPresentationStyle = .fullScreen
|
추가적으로 이펙티브자바의 item46(제가 정리한 글입니다 ㅎㅎ)에서는 FP 코드를 작성할때 SideEffect가 없도록 작성하라고 얘기하네요. 즉 외부값을 참조해서 변화시키면 안된다고 얘기가 나옵니다. |
|
다시 리뷰를 보니 코드 컨벤션이나 네이밍 관련해서 빡세게 리뷰한 듯 합니다;; 😅
다만 ViewModel이 Presenter로서의 역할을 해야한다는 것만 기억하셨으면 좋겠습니다. 테스트도 하면 좋습니다. |
@ehgud0670
안녕하세요 제이슨!😊
PR 요청드립니다. 잘 부탁드립니다.🙏
주요 작업
고민한 것
Delegate 패턴
Custom Alert View 구현
해결하지 못한 것