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 1] yujeong #39

Open
wants to merge 3 commits into
base: rft_3_yujeong
Choose a base branch
from

Conversation

yujeong-kwon
Copy link

@uuu1101
안녕하세요!
이번 미션에서는 ViewController 클래스의 책임이 많은 것 같아서 protocol을 이용해서 분리하였고
내용이 많은 메서드 들도 분리해서 구현해봤습니다
또한 그런 과정에서 ViewController 와 View도 나눠서 View 부분에서는 View에 보여지는 부분에 대한 처리를 하도록 분리하였습니다

조언을 얻고싶은 부분

  • func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell 함수에서 더 쪼갤 수는 없을지 궁급합니다
  • 미션 내용 중 "불필요한 코드와 중복되는 코드" 라는게 어떤 코드들인지 잘 이해가 되지 않았습니다
  • VIewCotroller에서 추상화되지않은 구체타입(WeatherDetailViewController)에 의존하고 있다고 생각하여 프로토콜을 활용해서 의존성을 낮춰야겠다고 생각했는데 어떤식으로 구현이 가능할지 조언을 좀 얻고싶습니다..!

@uuu1101 uuu1101 self-requested a review April 23, 2024 06:37
Comment on lines 91 to 126
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell: UITableViewCell = tableView.dequeueReusableCell(withIdentifier: "WeatherCell", for: indexPath)

guard let cell: WeatherTableViewCell = cell as? WeatherTableViewCell,
let weatherForecastInfo = delegate?.getWeatherForecastInfo(row: indexPath.row) else {
return cell
}

setCellLabel(cell: cell, weatherForecastInfo: weatherForecastInfo)

let iconName: String = weatherForecastInfo.weather.icon
let urlString: String = "https://openweathermap.org/img/wn/\(iconName)@2x.png"


if let image = delegate?.getImageChacheObject(urlString: weatherForecastInfo.weather.icon) {
cell.weatherIcon.image = image
return cell
}

Task {
guard let url: URL = URL(string: urlString),
let (data, _) = try? await URLSession.shared.data(from: url),
let image: UIImage = UIImage(data: data) else {
return
}

delegate?.setImageChacheObject(image: image, urlString: urlString)

if indexPath == tableView.indexPath(for: cell) {
cell.weatherIcon.image = image
}

}

return cell
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
let cell: UITableViewCell = tableView.dequeueReusableCell(withIdentifier: "WeatherCell", for: indexPath)
guard let cell: WeatherTableViewCell = cell as? WeatherTableViewCell,
let weatherForecastInfo = delegate?.getWeatherForecastInfo(row: indexPath.row) else {
return cell
}
setCellLabel(cell: cell, weatherForecastInfo: weatherForecastInfo)
let iconName: String = weatherForecastInfo.weather.icon
let urlString: String = "https://openweathermap.org/img/wn/\(iconName)@2x.png"
if let image = delegate?.getImageChacheObject(urlString: weatherForecastInfo.weather.icon) {
cell.weatherIcon.image = image
return cell
}
Task {
guard let url: URL = URL(string: urlString),
let (data, _) = try? await URLSession.shared.data(from: url),
let image: UIImage = UIImage(data: data) else {
return
}
delegate?.setImageChacheObject(image: image, urlString: urlString)
if indexPath == tableView.indexPath(for: cell) {
cell.weatherIcon.image = image
}
}
return cell
}
func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {
guard let cell = tableView.dequeueReusableCell(withIdentifier: "WeatherCell", for: indexPath) as? WeatherTableViewCell,
let weatherForecastInfo = delegate?.getWeatherForecastInfo(row: indexPath.row) else {
return UITableViewCell()
}
setCellLabel(cell: cell, weatherForecastInfo: weatherForecastInfo)
updateWeatherIcon(for: cell, at: indexPath, with: weatherForecastInfo.weather.icon)
return cell
}
func updateWeatherIcon(for cell: WeatherTableViewCell, at indexPath: IndexPath, with iconName: String) {
let urlString = "https://openweathermap.org/img/wn/\(iconName)@2x.png"
if let image = delegate?.getImageChacheObject(urlString: iconName) {
cell.weatherIcon.image = image
return
}
Task {
guard let url = URL(string: urlString),
let (data, _) = try? await URLSession.shared.data(from: url),
let image = UIImage(data: data) else {
return
}
delegate?.setImageChacheObject(image: image, urlString: iconName)
if tableView.indexPath(for: cell) == indexPath {
cell.weatherIcon.image = image
}
}
}

이런식으로 쪼개 볼 수 있을것 같긴하네요.

Comment on lines 128 to 139
func setCellLabel(cell: WeatherTableViewCell, weatherForecastInfo: WeatherForecastInfo) {
cell.weatherLabel.text = weatherForecastInfo.weather.main
cell.descriptionLabel.text = weatherForecastInfo.weather.description
cell.temperatureLabel.text = "\(weatherForecastInfo.main.temp)\( delegate?.getTempUnit() ?? "")"

let date: Date = Date(timeIntervalSince1970: weatherForecastInfo.dt)
cell.dateLabel.text = delegate?.convertDateToString(date: date)
}

func setCellImage(cell: WeatherTableViewCell, weatherForecastInfo: WeatherForecastInfo) {

}
Copy link
Member

Choose a reason for hiding this comment

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

윗 부분에서 나눈것과 별개로 셀의 어떠한 부분들을 변경하는것이라면 Cell이 해당 메서드를 가지는게 좋을것 같아 보입니다.
셀에는 필요한 데이터들만 넘겨주고, 셀에서 직접 변경하도록 말이죠


var delegate: WeatherViewDelegate?

func setTableView(){
Copy link
Member

Choose a reason for hiding this comment

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

set보다는 configure 같은 네이밍은 어떠신가요?

func refreshNavigationTitle(title: String)
}

final class WeatherInfo {
Copy link
Member

Choose a reason for hiding this comment

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

Class의 이름이 모델처럼 느껴져서, JSON을 fetch하는 객체가 느껴지도록 하면 좋을 것 같아요!

print(error.localizedDescription)
return nil
}
delegate.refreshNavigationTitle(title: info.city.name)
Copy link
Member

Choose a reason for hiding this comment

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

fetchWeatherJSON와는 관련이 없는 로직인것 같은데 따로 메서드로 빼보면 어떨까요?
컴플리션 핸들러를 통해서 WeatherForecastViewController 에서도 사용가능할것 같기도 하네요.

@yujeong-kwon
Copy link
Author

@uuu1101
안녕하세요!
결국 모든 미션을 완료하지는 못했지만 늦게나마 리뷰해주신 부분 수정하여 올립니다

프로토콜 관련한 부분은 아직 익히는 중이라 시간안에 적용은 못했지만 리뷰해주신대로 혼자서라도 꼭 적용해보겠습니다!! 감사합니다

  • 테이블뷰에 cellForRowAt 함수를 더 쪼개고 cell의 내용을 바꾸는 메서드는 cell에서 수행하도록 수정했습니다
  • async/await 를 사용하여 네비게이션의 타이틀 수정하는 함수를 분리했습니다

Copy link
Member

@uuu1101 uuu1101 left a comment

Choose a reason for hiding this comment

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

간단한 코멘트 몇가지 남겼습니다. 3주간 고생많으셨습니다 !

func refreshNavigationTitle(title: String)
}

final class WeatherInfo {
final class FetchWeatherInfo {
Copy link
Member

Choose a reason for hiding this comment

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

데이터 타입에 동사형이 들어가서 함수로 생각될 수 있을것 같아요. 오히려 기존의 WaetherInfo가 더 좋은 네이밍 같습니다!

@@ -36,7 +36,14 @@ final class WeatherInfo {
print(error.localizedDescription)
return nil
}
delegate.refreshNavigationTitle(title: info.city.name)
Task { @MainActor in
Copy link
Member

Choose a reason for hiding this comment

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

MainActor도 사용하셨군요 ! 👍

@@ -100,4 +100,15 @@ class WeatherTableViewCell: UITableViewCell {
weatherLabel.text = "~~~"
descriptionLabel.text = "~~~~~"
}

func setCellLabel(weatherForecastInfo: WeatherForecastInfo, tempUnit: String, dateStr: String) {
Copy link
Member

Choose a reason for hiding this comment

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

많이 길지 않아서 dateStr 라고 축약하는것 보단 dateString 이 더 좋은것 같습니다.

Comment on lines +98 to +99
setCellLabel(for: cell, with: weatherForecastInfo)
updateWeatherIcon(for: cell, at: indexPath, with: weatherForecastInfo)
Copy link
Member

Choose a reason for hiding this comment

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

메서드로 잘 나눠 주셨네요 👍🏻

func setCellLabel(for cell: WeatherTableViewCell, with weatherForecastInfo: WeatherForecastInfo) {
let tempUnit = delegate?.getTempUnit() ?? ""
let date: Date = Date(timeIntervalSince1970: weatherForecastInfo.dt)
let dateStr = delegate?.convertDateToString(date: date) ?? ""
Copy link
Member

Choose a reason for hiding this comment

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

앞서 드린 리뷰내용과 동일합니다 dateStr

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