Skip to content
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

날씨앱 [STEP 2] hyujin #42

Open
wants to merge 7 commits into
base: rft_3_hyujin
Choose a base branch
from

Conversation

h-yujin
Copy link

@h-yujin h-yujin commented Apr 25, 2024

@havilog
STEP2 부분은 기존 STEP1이랑 많이 겹치는 것 같아
화씨/섭씨 변경과 MVVM 구조 변경을 중점적으로 리팩토링하였습니다.

조언을 얻고 싶은 부분

ViewController과 View, ViewModel 사이의 바인딩을 제대로 한것인지,
ViewModel에는 비즈니스로직, View와 ViewController는 UI 로 나름 구분하여 개발하였으나, refreshControl의 위치가 모호하였습니다.
또, WeatherView를 ViewModel이 갖고있다보니 UITableViewDelegate>didSelectRowAt이 호출되면
WeatherView에서 WeatherViewModel 그리고 WeatherViewController로 액션을 넘겨주는 구조로 짜게 되었는데 이것이 쓸데없는 여러 과정을 거치는 것이 아닌가 하는 생각도 듭니다.
구현한 부분을 확인하고 조언 부탁드리겠습니다!

Copy link

@havilog havilog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

포팅하느라 고생 많으셨어요!
뷰 - 뷰모델의 역할에 대해 코멘트 드린 내용 확인 부탁드려요!

Comment on lines +29 to +30
viewModel.process(.loadView)
bindingView()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadView 시점에는 self.view를 갈아끼는 행동 이외에 다른 행동은 지양해주세요!
viewModel과 view를 바인딩하는 시점은 viewDidLoad에서 수행하는걸 권장합니다!

init(api: WeatherServiceable) {
self.api = api
viewModel = WeatherViewModel(api: api)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

뷰모델 또한 생성자 주입으로 받으면 좋을 거 같아요!


private var weatherView: WeatherView?
private var api: WeatherServiceable
let viewModel: WeatherViewModel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let viewModel: WeatherViewModel
private let viewModel: WeatherViewModel

await fetchWeatherAPI()
refreshControl.endRefreshing()
private func bindingView() {
view = viewModel.view
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

뷰모델은 뷰에 관한건 알지 못하는게 좋아요!
즉 뷰를 만들고, 어떤 뷰가 생성/삭제되는지는 뷰컨에서 수행하고,
뷰모델은 비지니스 로직을 수행해서 어떠한 상태를 변경하는 행위만 해주세요

Comment on lines +19 to +24
@Published var view: WeatherView?
@Published var cityName: String?
@Published var currentUnitTitle: String?

@Published var didSelectAction: ((WeatherForecastInfo, City) -> Void)?
@Published var networkErrorMessage: String?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private(set)을 붙여주면 좋을 것 같아요!

// Created by Hong yujin on 4/25/24.
//

import UIKit
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

극단적으로 뷰모델 파일에는 UIKit을 import하면 안된다고 생각해주세요

case refresh
}

private let refreshControl: UIRefreshControl = UIRefreshControl()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요친구는 뷰컨으로 가서 뷰컨에서 addView, autolayout을 잡고,
refresh가 땡겨졌는지에 대한 행동을 뷰컨에서 탐지해서 뷰모델로 올려주세요!


private let refreshControl: UIRefreshControl = UIRefreshControl()

@Published var view: WeatherView?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요친구도 뷰컨으로!

Comment on lines +49 to +57
private func loadView() {
view = WeatherView(delegate: self)

refreshControl.addTarget(self,
action: #selector(refresh),
for: .valueChanged)

view?.setTableViewRefreshControl(refreshControl: refreshControl)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요친구도 뷰컨으로!

행동: view -> viewcontroller -> viewmodel
상태 변경: viewmodel -> dependency(네트워크, 복잡한 로직을 처리하는 어떤 객체 - 템프 유닛, 타이머, 데이터베이스 등등) -> viewmodel 상태 변경
상태 관찰: viewmodel -> viewcontroller -> view

요런 순서로 진행된다고 보심 될 거 같아요

참고 유튜브 : https://youtu.be/M58LqynqQHc?si=rTAvM3-GGr-fIvCo


private func fetchWeatherAPI() async {
do {
let weatherJSON = try await api.fetchWeatherJSON()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API에서 온 resposne를 viewcontroller에 퍼블리시 -> 뷰컨에서 옵져빙 -> 뷰의 상태 변경

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.

2 participants