날씨앱 [STEP 1] kun #15
날씨앱 [STEP 1] kun #15
Conversation
There was a problem hiding this comment.
해결이 되지 않은 점
코멘트에 남겼습니다!
MainWeatherListViewController 클래스에서 MainWeatherListView 클래스를 분리해내면서 날씨 json 데이터를 가져오는 함수(refresh())를 MainWeatherListView 클래스에서 실행을 하게 되는데 이부분을 MainWeatherListViewController 클래스에서 실행 후 가공한 데이터를 View로 넘겨주는 게 더 나은지 하비의 의견이 궁금합니다!
MainWeatherListViewController를 분리한 것은 Controller가 mvvm에서 뷰모델의 역할 즉 비지니스 로직 관리를 하고, View가 뷰를 보여주기 위한 역할을 하는거라고 이해했는데요,
json을 가져오고, 포매팅 하는 것은 비지니스 로직에 해당할거 같아요
따로 수정해줘야할 필요를 못느껴 WeatherJSON 타입과 같이 DTO 모델을 따로 데이터모델로 가공하지 않고 사용하고 있는데 이렇게도 많이 사용하는지 궁금합니다!
코멘트에 단 것처럼 오히려 computed property를 사용해서 복잡도가 올라가지 않았나 하는 느낌이 들어요!
사실 DTO까지는 필요없다 라는 생각은 동의하는데, 그렇다면 그냥 접근제어를 풀어서 뷰에서 모델 프로퍼티에 접근하는게 복잡도가 떨어질 것 같습니다!
| import Foundation | ||
|
|
||
| // MARK: - Temperature Unit | ||
| enum TempUnit: String { |
There was a problem hiding this comment.
rawValue를 사용하는 곳은 없어보이는데, String으로 선언하신 이유�는 무엇일까요?
| } | ||
| } | ||
|
|
||
| var strOpposite: String { |
There was a problem hiding this comment.
이 변수명만 보고 이 변수가 어떤 값을 반환하는지 추측하기 어려워보여요
| var date: String { weatherForecastInfo.date } | ||
|
|
||
| var sunrise: String { cityInfo.sunriseString } | ||
| var sunset: String { cityInfo.sunsetString } | ||
|
|
||
| var weatherMain: String { weatherForecastInfo.weatherMain } | ||
| var description: String { weatherForecastInfo.description } | ||
|
|
||
| var temp: String { "\(weatherForecastInfo.temp)\(tempUnit.expression)"} | ||
| var tempMax: String { "\(weatherForecastInfo.tempMax)\(tempUnit.expression)" } | ||
| var tempMin: String { "\(weatherForecastInfo.tempMin)\(tempUnit.expression)" } | ||
|
|
||
| var feelsLike: String { "\(weatherForecastInfo.feelsLike)\(tempUnit.expression)" } | ||
| var pop: String { weatherForecastInfo.pop } | ||
|
|
||
| var humidity: String { weatherForecastInfo.humidity } | ||
|
|
||
| var iconName: String { weatherForecastInfo.iconName } |
There was a problem hiding this comment.
이 computed property들을 만드신 이유가 궁금해요.
Clean-Architecture에서는 서버에서 받은 모델을 뷰에서 사용할 모델로 새로 만드는 역할이 있는데요, DTO, Entity키워드로 검색해보시면 좋을 거 같고,
해당 레이어가 빠진다고 하더라도, weatherForecastInfo를 internal로 열어서 직접 접근하는게, computed property를 사용하는것보다 오버헤드가 적을거 같다는 생각이 드네요
| enum JsonError: Error { | ||
| case emptyData // 데이터 미존재 | ||
| case failDecode // 디코드 실패 | ||
| } |
There was a problem hiding this comment.
CustomStringConvertible 프로토콜로 주석에 있는 설명을 description으로 옮겨보는건 어떨까요?
|
|
||
| import UIKit | ||
|
|
||
| final class WeatherJsonService: JsonService { |
There was a problem hiding this comment.
- JsonService의 경우 WeatherJSON, JsonError에만 특정된 fetch를 할 수 있을거 같아요. 다만 JsonService를 선언하신 목적은 json에서 무언가를 파싱하는 기능을 선언하신 거 같고, 확장성을 가지려면 제네릭한 Return Type을 가지는게 좋아보여요
- Asset에서 json을 가져오는 행동은 동기로 일어나는데, async인 이유는 무엇일까요?
- throws함수로 만들 수도 있을 거 같은데, 굳이 Result로 변환하시는 이유는 무엇일까요?
- JSONDecoder의 경우 외부에서 주입받는것도 확장성을 위해 고려할 수 있을 것 같아요
|
|
||
| import UIKit | ||
|
|
||
| final class WeatherDetailViewController: UIViewController { |
There was a problem hiding this comment.
의존성을 분리하고 객체를 분리하는건 마땅히 해야할 좋은 일이라고 생각하는데요,
사실 지금 DetailViewController에서는 뷰를 보여주는 일만 하고 있는거 같아요.
init 시점에 WeatherDetailInfo만 받아서 뿌려주면 될 거 같은데, 오버 엔지니어링은 아닐까 하는 생각이 드네요.
쿤은 어떻게 생각하시나요?
| private func updateIcon() { | ||
| Task { | ||
| iconImageView.image = try? await dependency.imageService.fetchImage(iconName: dependency.weatherDetailInfo.iconName, urlSession: URLSession.shared) | ||
| } |
There was a problem hiding this comment.
이 부분도 위에 말한것 처럼 UIImageView에 extension으로 이미지 로드를 관리하는 객체(이미지 캐시와, 이미지 가져오는 기능을 가진)를 넣어주는건 어떨까요?
| private let dependency: Dependency | ||
| private var weatherJSON: WeatherJSON? | ||
|
|
||
| var delegate: MainWeatherListViewDelete? |
| private func fetchWeather() async -> Result<WeatherJSON, JsonError> { | ||
| return await dependency.weatherJsonService.fetchWeather() | ||
| } |
There was a problem hiding this comment.
재사용되는 함수가 아니라면, 한줄짜리 함수는
오히려 코드 점핑이 일어나서 가독성이 떨어질 수도 있을거 같아요
| func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) { | ||
| tableView.deselectRow(at: indexPath, animated: true) | ||
|
|
||
| let weatherDetailVC = createWeatherDetailViewController(indexPath: indexPath) | ||
|
|
||
| delegate?.showWeatherDetailInfo(detailVC: weatherDetailVC as! WeatherDetailViewController) | ||
| } | ||
| } |
There was a problem hiding this comment.
didSelectRowAt이 불리면, 해당 셀에 있는 정보만 상위 delegate에 전달해주면 될 것 같은데 역할이 너무 많은 느낌이 드네요!
@havilog
안녕하세요 하비!
늦게 날씨앱 PR 보내게 됐네요...
해결이 되지 않은 점
TempUnitManagerService프로토콜을 채택하는TempUnitManager클래스를 두어tempUnit프로퍼티와update메서드를 두고 있고TempUnitenum 값 내부에expression과strOpposite프로퍼티를 두어 사용하고 있습니다. enum 값 내부에 switch문을 사용하여 데이터를 가지고 있는 것을 수정하고 싶어 해당 데이터들을 모두TempUnitManager클래스에서 가지면서 unit 변경 시 해당 값들을 모두 수정되게 하며 관리하고 싶은데 어떤 식으로 설계를 해야할지 고민 중에 있습니다.조언을 얻고 싶은 부분
MainWeatherListViewController클래스에서MainWeatherListView클래스를 분리해내면서 날씨 json 데이터를 가져오는 함수(refresh())를MainWeatherListView클래스에서 실행을 하게 되는데 이부분을MainWeatherListViewController클래스에서 실행 후 가공한 데이터를 View로 넘겨주는 게 더 나은지 하비의 의견이 궁금합니다!따로 수정해줘야할 필요를 못느껴
WeatherJSON타입과 같이 DTO 모델을 따로 데이터모델로 가공하지 않고 사용하고 있는데 이렇게도 많이 사용하는지 궁금합니다!