-
Notifications
You must be signed in to change notification settings - Fork 17
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] 도라, 밤삭 #28
Conversation
…sion을 통한 기본구현메소드로 변경
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.
날씨 앱 Step1 잘 진행해보셨네요!
모델 타입을 꼼꼼하게 구현하시고,
apikey 보안에 대해서 생각해보신 점이 좋았습니다.👍
먼저 질문에 대한 제 생각을 적어보았습니다.
-
정답은 없는 것 같습니다. 개인의 생각에 따라 꼭 필요한 프로토콜이 될 수도 있고, 불필요할 수도 있지 않을까 싶습니다.
다만, 네트워크에 의존하지 않는 테스트 코드를 작성하기 위해 프로토콜 정의가 필요하다면 불필요한 작업이 아니라고 생각합니다!
오히려 외부 의존성이 필요한 기능(네트워크, DB, 라이브러리 등)에 직접 의존하지 않고 테스트 할 수 있는 방법을 연구하는 것도 좋을 것 같습니다! -
protocol을 사용하여 의존 관계를 역전할 필요는 없다고 생각합니다. 과도한 추상화는 프로그램 복잡도가 증가하고 가독성이 떨어질 수 있어 무조건적으로 사용하는 것은 지양하는 편입니다!
이외에도 여러 궁금증과 코멘트를 남겨보았습니다.
같이 의견 나눠보면 좋을 것 같습니다.😁
import Foundation | ||
|
||
protocol URLFormattable { | ||
associatedtype T: URLProtocol |
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.
URLFormattable
타입이 연관값으로 URLProtocol
을 가지게 하였을 때, 어떤 이점이 있을까요?
url
과 defaultPath
을 별개로 생각하고 서로 각각 재사용하기 위함인지 궁금합니다!
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.
protocol URLFormattable {
associatedtype T: APIBaseURLProtocol
func makeURL(path: String, with queries: [String: String]) -> URL?
}
extension URLFormattable {
func makeURL(path: String, with queries: [String: String]) -> URL? {
let urlString = "\(T.baseURLString)"
......
protocol APIBaseURLProtocol {
static var baseURLString: String { get }
}
struct WeatherURLFormatter: URLFormattable {
typealias T = WeatherURL
}
enum WeatherURL: APIBaseURLProtocol {
static let baseURLString = "https://api.openweathermap.org"
.....
현재는 이렇게 수정했습니다!
URLFormattable
을 채택하는 타입이 associatedtype
의 typealias
로 어떤 타입을 가지고 있게 하냐에 따라 URL을 포맷팅 해주는 역할을 프로토콜로 추상화를 통해 구현해보고 싶었습니다.
또한, 현 프로젝트에서는 하나의 API만 쓰이지만,
조금 더 확장성을 생각하여 어떠한 API에도 URL을 포맷팅해주는 역할을 구현해보고 싶었습니다.
그러기 위해 generic
을 사용해야 했는데, 프로토콜에서는 associatedtype
을 통해 해당 프로토콜이 채택하는 타입이 generic
과 유사하게 활용하는 방법을 택했습니다.
Protocol extension
을 통해 메소드 기본 구현하는 과정에서 포맷팅할 String 값에 접근하는 방법으로
associatedtype
에 APIBaseURLProtocol
로 제약을 주었습니다.
이 과정에서 APIBaseURLProtocol
를 따르는 객체는 타입프로퍼티로 해당 API의 baseURLString
을 가지고 있게하고, URLFormattable extension
의 makeURL
메소드가 baseURLString
값에 접근하여,
URL을 포맷팅해주는 기능을 구현하면 되겠다고 생각했습니다.
defaultPath는 재사용성이 없다고 판단하여 현재는 제거했습니다!
protocol URLFormattable { | ||
associatedtype T: URLProtocol | ||
|
||
var defaultPath: String {get} |
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.
다른 프로토콜과 컨벤션을 맞춰보는 것은 어떨까요?
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.
공백 수정하였습니다!
protocol URLProtocol { | ||
static var url: String { get } | ||
} |
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.
Swift
에서 제공하는 URLProtocol
타입과 이름을 동일하게 지정한 이유가 있을까요?
호스트 주소 url
까지 추상화한 이유를 알 수 있을까요?
self.session = session | ||
} | ||
|
||
func getData<T: Decodable>(path: String, with queries: [String: String], completion: @escaping (Result<T, Error>) -> Void) { |
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.
URLSession
을 이용하여 네트워크 요청 구문(generic, jsondecoder)을 잘 작성해보셨네요.👍
return completion(.failure(NetworkError.unknownError(description: error.localizedDescription))) | ||
} | ||
|
||
guard let response = response as? HTTPURLResponse else {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.
다른 guard
문에서 작성한 공백 컨벤션을 맞춰보는 것은 어떨까요?
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.
공백 수정하였습니다~!
var coord: Coord? | ||
var weather: [Weather]? | ||
var base: String? | ||
var main: Main? | ||
var visibility: Int? | ||
var wind: Wind? | ||
var clouds: Clouds? | ||
var rain: Rain? | ||
var snow: Snow? | ||
var dt: Int? | ||
var sys: Sys? | ||
var timezone: Int? | ||
var id: Int? | ||
var name: String? | ||
var cod: Int? |
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.
모든 프로퍼티를 옵셔널로 처리하신 이유가 궁금합니다.
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.
STEP1 진행 중이라 어떤 값들이 nil 값으로 들어올지 몰라 모든 프로퍼티를 옵셔널로 처리해두었습니다. 프로젝트 진행하며 확실한 값들은 옵셔널 처리를 하지 않는 방향으로 수정하겠습니다!
var lon: Double | ||
var lat: Double |
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.
API 문서에서 제공하는 단축어를 그대로 사용하기 보다는 전체 단어로 프로퍼티 이름을 지정해보는 것은 어떨까요?
의견이 궁금합니다!
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 Foundation | ||
|
||
struct WeeklyWeather: Decodable { |
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.
API의 모든 파라미터에 대한 모델을 구현해보셨네요.👍
|
||
import Foundation | ||
|
||
enum NetworkError: Error, CustomStringConvertible { |
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.
네트워크 에러 타입을 구현해보셨네요.👍
struct Weather: Decodable { | ||
var id: Int? | ||
var main: String? | ||
var description: String? | ||
var icon: String? | ||
} |
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.
모든 모델에 대한 타입이 전역적으로 선언되어 있네요.
Swift Nested Types
에 대해서 한번 알아보고 개선해봐도 좋을 것 같습니다.
지금도 좋습니다!
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.
WeeklyWeather, CurrentWeather 각각 따로 사용하는 모델은 Nested Type으로 변경하고,
공통으로 사용하는 모델은 NameSpace로 분리하도록 변경 했습니다!
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.
안녕하세요. 도라, 밤삭!
프로젝트를 더 개선해보셨네요. 좋습니다!👍
궁금증에 대한 의견을 달아주신 점도 좋았습니다!
Step1 진행하시느라 고생하셨습니다.
이번 스텝은 여기서 마무리하도록 하겠습니다.🎉
struct Coordinate: Decodable { | ||
var longitude: Double | ||
var latitude: Double | ||
|
||
enum CodingKeys: String, CodingKey { | ||
case longitude = "lon" | ||
case latitude = "lat" | ||
} |
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 Foundation | ||
|
||
enum CommonWeatherDTO { |
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.
전역적으로 선언된 모델을 모아서 구현해보셨네요.👍
날씨 앱
리뷰어
@zdodev
팀원:busts_in_silhouette:
프로젝트 파일 구조
프로젝트 설명
구현 내용
JSON
데이터와 매핑할 모델 설계CodingKeys
프로토콜 활용API
서버와 통신할 인스턴스 역할을 수행할 타입 구현URLSession
을 활용하여 구현각 역할 설명
Protocol
static 타입 프로퍼티
로url
을 변환하기 위한String
타입을타입 프로퍼티
로 가지고 있게 정의한 프로토콜 입니다.URLFormattable
을 채택하는 타입이URLProtocol
을 따르는 타입을Generic
으로 받아(타입 제한)URLProtocol
의static 타입 프로퍼티
의 값을 가지고URL타입으로 변환 시켜주는 역할
을 하게 해주는 프로토콜 입니다.associatedtype
을 사용하여 해당 프로토콜을 채택한 객체의Generic
으로URLProtocol
의 타입을 받아 의존을 객체간 직접 의존이 아닌 Protocol에 의존성을 갖게 했습니다.Model
NetworkManager
URLFormatable
프로토콜을 따르는 객체 와 URLSession을 내부 프로퍼티로 갖고 초기화 시점에 의존성 주입을 통해 초기화 하도록 하였습니다.urlFormatter
를 주입해주어URLProtocol
과URLFormattable
을 채택한 타입에 따라 생성되는 URL에 네트워크 통신이 가능하도록 하였습니다.NetworkManager
객체를 통해API
와URLSession
으로 통신할 수 있습니다.Extension
xcconfig
파일을 통해API KEY
를 숨기고info.plist
를 통해xcconfig
파일에서 정의한API KEY
값에 접근해서 사용할 수 있게 했습니다.Trouble Shooting
URLFormattable, URLFormatter, URLProtocol
첫번째 구현 방식
이렇게 하다보니 WeatherURL에만 국한된 프로토콜로 사용할 수 밖에 없었고.
URLFormattable 프로토콜 타입으로 객체 생성 후, makeURL을 호출했을 시,
파라미터의 urlType을 객체 초기화 시
any URLFormattable
타입으로 만들어지기 때문에 컴파일러가 추론할 수 없는 문제 발생하였습니다.두번째 구현 방식
WeatherURLFormatter객체 내부에 프로퍼티로 URLFormatter객체를 생성하여 만드는 방식으로 변경
최종 구현 방식
궁금한 점
Network Test
코드를 작성해 보고싶었습니다.그리하여
NetworkManager
를 채택하는Protocol
을 만들고,URLSession
을 상속받는MockURLSession
과URLSessionDataTask
를 상속받는MockURLSessionDataTask
를 만들어 테스트 코드를 작성해보려고 했는데, 두 클래스의init()
생성자가 iOS 13버전 이후로 deprecated 되었다는 컴파일러의 에러를 확인하였습니다.다른 방법을 확인해본 결과
URLSession
관련Protocol
과URLSessionDataTask
관련Protocol
을 만들어 각각 해당 프로토콜을 따르게 한후, 각각의 Mock을 만들어서UnitTest
를 작성하는 방법을 찾았는데,UnitTest
만을 위하여 해당 프로토콜을 만드는 것이 적당한 방법인지 아직 판단이 서질 않습니다.NetworkManager
가 만들어지지 않을 것 같은 판단이 드는데, 이러한 경우에도NetworkManager
가 채택할Protocol
을 구현하여NetworkManager
가 해당Protocol
을 따르게 한 후, 의존 관계를 역전 시켜주는 것이 좋은 지 고민이 됩니다.