-
Notifications
You must be signed in to change notification settings - Fork 25
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 1] Wan #22
base: rft_2_wannn
Are you sure you want to change the base?
날씨앱 [STEP 1] Wan #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요. @hsw1920 !
리뷰어 소대입니다!
날씨앱 Step1 잘 진행해주셨습니다!
요구사항에 대해 고민을 많이 하여 구현하려고 해보신 점이 좋았습니다!
역할과 책임에 대해서도 잘 나누고, 디렉터리 구조를 잘 만들어보신 것 같습니다.👍
몇 가지 코멘트를 남겨보았습니다.
확인해보시고 의견 나눠보면 좋을 것 같습니다!🙂
import Foundation | ||
import UIKit | ||
|
||
final class WeatherJSONService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewController
에서 데이터를 가져오는 역할을 분리해보셨네요.👍
WeatherJSONService
타입도 직접 참조하지 않고 DIP를 적용해보면 어떨까요!
import UIKit | ||
|
||
final class WeatherJSONService { | ||
func fetchWeather(completion: @escaping (WeatherJSON) -> ()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchWeather
메서드를 클로저를 사용하는 completion 방식으로 구현하신 이유가 궁금합니다!
do { | ||
info = try jsonDecoder.decode(WeatherJSON.self, from: data) | ||
} catch { | ||
print(error.localizedDescription) | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에러가 발생해서 앱이 동작하고 있지 않습니다! 기존의 기능을 유지하면서 코드를 수정해볼 수 있을까요?
|
||
import UIKit | ||
|
||
final class WeatherImageService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미지 캐시 역할을 분리해보셨네요.👍
WeatherImageService
타입도 직접 참조하지 않고 타입도 DIP를 적용해보면 어떨까요!
final class WeatherImageService { | ||
private let imageCache: NSCache<NSString, UIImage> = NSCache() | ||
|
||
func fetchImage(iconName: String, completion: @escaping (UIImage) -> ()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchImage
메서드를 클로저를 사용하는 completion 방식으로 구현하신 이유가 궁금합니다!
} | ||
|
||
override func loadView() { | ||
view = contentView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewController
와 View
를 분리해보셨네요.👍
contentView.weatherGroupLabel.text = listInfo.weather.main | ||
contentView.weatherDescriptionLabel.text = listInfo.weather.description | ||
contentView.temperatureLabel.text = "현재 기온 : \(listInfo.main.temp)\(weatherDetailInfo.tempUnit.symbol)" | ||
contentView.feelsLikeLabel.text = "체감 기온 : \(listInfo.main.feelsLike)\(weatherDetailInfo.tempUnit.symbol)" | ||
contentView.maximumTemperatureLable.text = "최고 기온 : \(listInfo.main.tempMax)\(weatherDetailInfo.tempUnit.symbol)" | ||
contentView.minimumTemperatureLable.text = "최저 기온 : \(listInfo.main.tempMin)\(weatherDetailInfo.tempUnit.symbol)" | ||
contentView.popLabel.text = "강수 확률 : \(listInfo.main.pop * 100)%" | ||
contentView.humidityLabel.text = "습도 : \(listInfo.main.humidity)%" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewController
와 View
의 프로퍼티에 직접 접근하여 값을 설정하도록 하신 이유가 있을까요?
보다 느슨하게 결합해보는 것은 어떨까요?🙂
@objc private func refresh() { | ||
tableView.reloadData() | ||
refreshControl.endRefreshing() | ||
navigationItem.title = weatherJSON?.city.name | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tableView
를 리로드 해도 새로운 데이터를 받아오지 않고 있습니다! 기존의 기능을 유지하면서 코드를 수정해볼 수 있을까요?
cell.configure(weatherForecastInfo: weatherForecastInfo, | ||
tempUnit: tempUnit, | ||
imageService: imageService) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewController
에서 셀을 직접 수정하지 않고, 뷰 데이터를 전달하도록 수정해보셨네요.👍
} | ||
} | ||
|
||
extension WeatherViewController: UITableViewDataSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UITableViewDataSource
역할을 ViewController
가 수행하는 것으로 보입니다.
꼭 ViewController
에서 수행해야 하는 것이 아니라면 UITableViewDataSource
역할을 누가 수행하면 좋을까요? 역할과 의존성의 관점에서 고민해보면 좋을 것 같습니다!
@zdodev
소대 안녕하세요! 완입니다 :)
날씨 앱 Step1 PR 올립니다!
학기 시작과 개인 일정이 겹쳐서 week2를 더 열심히 하지 못 한것 같아 아쉬운 생각이 들지만 잘 부탁드립니다 :)
미션 요구사항
내용
기존 ViewController와 DetailViewController의 경우
view를 보여주거나 액션, 관련 데이터의 갱신, JSON 파싱, image관련 캐싱, 패치 등의 역할이 굉장히 많이 혼잡해있었습니다.
SRP와 6원칙에 집중하여 이를 분리시켜 ImageService, JSONService로 분리하고 ViewController에서 View를 분리하였습니다.
또한 최상위 WeatherJSON의 경우 쓰임새가 많고 하위 Weather관련 모델의 최상위로 취급하여 따로 분리하여 캡슐화를 하였습니다.
그리고 JSON파싱과 image 네트워킹 관련 서비스를 만들었으며,
Cache인스턴스를 공유하기 위해 SceneDelegate에서 imageService를 주입시켜 사용하였습니다.
조언을 얻고 싶은 부분