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] jaySong #41

Open
wants to merge 2 commits into
base: rft_3_jaysong
Choose a base branch
from

Conversation

jysong91
Copy link

ISP, DIP를 어디에 적용해야할지 잘 모르겠습니다.

그리고, 전역변수의 옵셔널 언래핑을 하고 메서드의 파라미터로 주는 게 나을지, 아니면 메서드에서 언래핑을 하는게 나을지 고민이 됐습니다.

감사합니다.

@zdodev

@jysong91
Copy link
Author

step2, bonus까지 push 했는데, 이렇게 하면 될까요?

Copy link

@zdodev zdodev left a comment

Choose a reason for hiding this comment

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

안녕하세요. @jysong91 !

날씨 앱 리팩터링도 잘 진행해주셨네요!
여러 책임을 나눠보고 메서드를 작게 나눠보신 점이 좋았습니다.👍

몇 가지 코멘트를 남겨보았습니다.
같이 의견 나눠보면 좋을 것 같습니다!

Comment on lines +10 to +13
final class ImageCache {
static let shared: NSCache<NSString, UIImage> = NSCache()
private init() {}
}
Copy link

Choose a reason for hiding this comment

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

ViewController에서 Cache 기능을 분리해보셨네요. 좋습니다!👍
다른 타입 정의와 분리해서 새로운 파일에서 작성해보는 것은 어떨까요?

private init() {}
}

class WeatherListViewController: UIViewController {
Copy link

Choose a reason for hiding this comment

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

ViewController의 이름을 잘 바꿔보셨네요.👍

Comment on lines +80 to 82
extension WeatherListViewController {
private func fetchWeatherJSON() {

Copy link

Choose a reason for hiding this comment

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

ViewController에서 JSON 데이터를 가져오는 기능도 수행하는 것으로 보입니다.
해당 책임을 다른 곳으로 분리해보는 것은 어떨까요? 객체지향 원칙을 지키는 데 더 도움이 될 것 같습니다!

Comment on lines +100 to +106
private func setData(cell: WeatherTableViewCell, data: WeatherForecastInfo) {
cell.weatherLabel.text = data.weather.main
cell.descriptionLabel.text = data.weather.description
cell.temperatureLabel.text = "\(data.main.temp.changeTemperature(by:tempUnit))\(tempUnit.expression)"
cell.dateLabel.text = dateFormatter.string(from: data.dt)
cell.weatherIcon.setImage(from: data.weather.icon)
}
Copy link

Choose a reason for hiding this comment

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

ViewController가 직접 cell의 데이터를 설정하기 보다는 cell이 직접 설정할 수 있도록 책임을 지어주는 것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

cell에 func setData(data:) 를 만들어주면 되겠네요

}
}

extension WeatherListViewController: UITableViewDelegate {
Copy link

Choose a reason for hiding this comment

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

현재 WeatherListViewController 타입이 UITableViewDataSource 책임을 가지고 있지만, ViewController에서 DataSource 책임을 가지지 않고 분리하는 것도 생각해볼 수 있을 것 같습니다.
ViewController의 책임을 줄여보는 것은 어떨까요? 다른 타입을 생성하는 것도 좋은 방법이 될 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

UITableView를 상속하는 CustomTableView 타입을 만드는 방법은 어떨까요?

Copy link

Choose a reason for hiding this comment

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

UITableView를 상속하는 CustomTableView 타입을 생성하는 해서 역할을 분리하는 것도 괜찮은 방법이 될 것 같습니다!

}
}

extension UIImageView {
Copy link

Choose a reason for hiding this comment

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

기본 타입에 대한 extension은 새로운 파일을 생성해서 관리해보는 것은 어떨까요?

Comment on lines +161 to 164
if let cache = cache, let image = cache.object(forKey: urlString as NSString) {
self.image = image
return
}
Copy link

Choose a reason for hiding this comment

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

캐시 타입에서 캐시된 데이터를 저장하고, 다시 불러오는 역할을 수행하는 메서드를 정의해보는 것은 어떨까요?
지금보다 가독성이 더 좋아질 것 같습니다!

Copy link
Author

@jysong91 jysong91 Apr 27, 2024

Choose a reason for hiding this comment

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

loadCache 같은 이름으로 분리하면 좋을 것 같네요!

Comment on lines 55 to +58
var expression: String {
switch self {
case .metric: return "℃"
case .imperial: return "℉"
}
guard let expression = expressionMapping[self] else { return ""}
return expression
}
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 25 to 27
var weatherForecastInfo: WeatherForecastInfo?
var cityInfo: City?
var tempUnit: TempUnit = .metric
Copy link

Choose a reason for hiding this comment

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

외부에서 직접 접근해서 데이터를 받을 수도 있지만, 직접 접근이 아닌 방식으로 데이터를 초기화할 수 있도록 해보면 좋을 것 같습니다! DI와 같은 키워드를 확인해보면 좋을 것 같습니다!

Comment on lines +37 to 38
// 옵셔널 언래핑을 해서 파라미터로 전달하는게 나을지, 아니면 각 메서드 안에서 하는게 나을지
guard let listInfo = weatherForecastInfo else { return }
Copy link

Choose a reason for hiding this comment

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

언래핑 시점이 언제 적당할 지 고민해보면 좋을 것 같습니다! 그리고 어떤 상황에서 guard문을 사용할지, if let문을 사용할지 부분도 생각해보면 좋을 것 같습니다!
두 문법 다 언래핑을 지원하지만 guard문의 경우 언래핑 이전에 조기 종료 역할을 가지고 있어서 상황에 맞는 방식을 적용하면 될 것 같습니다!

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