날씨앱 [STEP 1] garden #12
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
havilog
suggested changes
Feb 5, 2024
WeatherForecast/WeatherForecast/WeatherDetailViewController.swift
Outdated
Show resolved
Hide resolved
WeatherForecast/WeatherForecast/WeatherDetailViewController.swift
Outdated
Show resolved
Hide resolved
havilog
reviewed
Feb 8, 2024
Comment on lines
50
to
71
| class ImageCacheManager { | ||
| static let shared = ImageCacheManager() // 싱글턴 인스턴스 | ||
| private var imageCache: NSCache<NSString, UIImage> = NSCache() | ||
|
|
||
| func cacheImage(_ image: UIImage, forKey key: String) { | ||
| imageCache.setObject(image, forKey: key as NSString) | ||
| log("Image cached for key: \(key)") | ||
| } | ||
|
|
||
| func getImage(forKey key: String) -> UIImage? { | ||
| if let cachedImage = imageCache.object(forKey: key as NSString) { | ||
| log("Returning cached image for key: \(key)") | ||
| return cachedImage | ||
| } | ||
| log("No cached image : \(key)") | ||
| return nil | ||
| } | ||
|
|
||
| private func log(_ message: String) { | ||
| print(message) | ||
| } | ||
| } |
There was a problem hiding this comment.
- 상속이 안되는 class의 경우 final을 default로 붙여주세요!
- imageCache는 let이어도 될 것 같아요!
- 디스크 캐시가 생길수도 있을거 같아서 protocol로 캐시할 수 있는 기능을 추상화 해보는것도 좋을 것 같아요
| // Created by 김정원 on 2/4/24. | ||
| // | ||
| import UIKit | ||
| final class TransforJSON { |
There was a problem hiding this comment.
- JSON을 가져오는 기능과, 이미지를 가져오는 기능은 분리되면 좋을 것 같아요
- jsonDecoder를 파라미터로 주입받아서 확장성을 키워보는것도 좋을 것 같아요
- fetch�Json<T: Decodable>() throws -> T 와 같이 작성한다면, 디코딩에서의 에러도 받을 수 있고 여러가지 json을 디코딩 할 수 있을것 같아요
WeatherForecast/WeatherForecast/WeatherDetailViewController.swift
Outdated
Show resolved
Hide resolved
WeatherForecast/WeatherForecast/WeatherForecastViewController.swift
Outdated
Show resolved
Hide resolved
WeatherForecast/WeatherForecast/WeatherForecastViewController.swift
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 !! @havilog
고민했던 점
데이터를 불러오는 부분을 따로 분리하면 좋을 것 같다고 생각해 네트워크 전용 클래스를 만들었습니다.
weather , city , tempunit 를 현재는 각자 따로 받아오고 있는데 한번에 전부 보내서 하위 모듈에서 수정하는게 나은지 상위 모듈에서 3개를 분리해주는게 나은지 고민입니다.
label 이나 image 를 전역으로 설정했는데 이 부분 피드백 부탁드려요 !