-
Notifications
You must be signed in to change notification settings - Fork 33
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 2] hoon, MINT #110
base: ic_9_mint09
Are you sure you want to change the base?
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.
수고하셨어요 훈트!
전체적인 일정을 잡고, 네트워크의 본질적인 부분에 대해 고민하느라 앞쪽에서 시간을 많이 소비해서 코드적으로 다른 캠퍼들에 비해 많이 못봐드려서 아쉽네요 ㅠㅠ
하지만 절대 무의미한 시간은 아니었을거라고 생각되어요!
다른 네트워크 구현체들 보고 참고해서
전체적으로 어떤 부분을 놓치고 있었는지,
확장성 있는 네트워크가 되려면 어떻게 해야할 지
고민해보시면 좋을 듯 싶습니다!
.DS_Store
Outdated
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.
dsstore가 올라와 있네요!
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.
수정하였습니다.😊
BoxOffice/Extension/Decodable+.swift
Outdated
extension Decodable { | ||
static func decode(data: Data) -> Self? { | ||
var result: Self? | ||
|
||
do { | ||
result = try JSONDecoder().decode(Self.self, from: data) | ||
} catch { | ||
print(error.localizedDescription) | ||
} | ||
|
||
return result | ||
} | ||
} |
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.
이 함수의 존재의 이유를 잘 모르겠어요!
decode에 실패했을 때 print를 찍는건 적절한 에러 핸들링이 아닐 거 같다는 생각이 듭니다 ㅠㅠ
struct MovieInformation: Decodable { | ||
let movieInformationResult: MovieInformationResult | ||
|
||
enum CodingKeys: String, CodingKey { |
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.
enum CodingKeys: String, CodingKey { | |
private enum CodingKeys: String, CodingKey { |
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.
접근 제어를 사용하여 외부에서 접근하지 못하도록 수정하였습니다.
관련 커밋: 8c8af63
BoxOffice/Network/HTTP.swift
Outdated
var typeName: String { | ||
switch self { | ||
case .get: | ||
return "GET" | ||
case .put: | ||
return "PUT" | ||
case .post: | ||
return "POST" | ||
case .patch: | ||
return "PATCH" | ||
case .delete: | ||
return "DELETE" | ||
} | ||
} |
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.
HTTPMethod의 rawValue를 안쓰고 typeName을 정의해주신 이유가 있을까요?
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.
rawValue를 사용하게 되면 실제로 해당 enum을 사용할 때 어떤 타입을 사용하고 있는지 불명확하다는 생각에 연산 프로퍼티로 정해주었습니다.
|
||
import Foundation | ||
|
||
struct NetworkManager { |
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.
NetworkManager를 struct로 구현하신 이유에 대해 설명해주시면 좋을 것 같아요!
BoxOffice/Network/URLType.swift
Outdated
guard let url = component.url else { | ||
fatalError(NetworkError.invalidURL.localizedDescription) | ||
} |
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.
fatalError를 핸들링 할 수 있으면 좋을거 같아요
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.
fetalError를 핸들링한다는 말이 혹시 do-catch가 될 수 있게 바꾸는 것을 말씀 하시는 걸까요? 아니면 사용자에게 보이도록 알람과 같은 기능을 띄우는 것을 의미하시는 걸까요?
BoxOffice/Network/URLType.swift
Outdated
static let key = URLQueryItem(name: "key", value: "f5eef3421c602c6cb7ea224104795888") | ||
static var date: URLQueryItem? | ||
static var code: URLQueryItem? | ||
|
||
static func selectDate(_ date: String) { | ||
self.date = URLQueryItem(name: "targetDt", value: date) | ||
} | ||
|
||
static func selectMovieCode(_ code: String) { | ||
self.code = URLQueryItem(name: "movieCd", value: code) | ||
} |
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.
공통적으로 쓰이는 key값이 있는건 괜찮을 거 같지만,
만약
- 로그인하는 api
- 유저 정보를 가져오는 api
- 영화 정보를 가져오는 api
등이 있을 때 이 Query가 어떻게 확장될 수 있을지 고민해보시면 좋을 것 같습니다!
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.
여러 주소에서 사용할 수 있도록 URLQuery를 protocol로 구현하였습니다.
관련 커밋: bed67b1
BoxOffice/Network/URLType.swift
Outdated
} | ||
} | ||
|
||
private func configureURL(path: String, query: [URLQueryItem?]) -> URL { |
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.
요 아이도 현재는 get만 고려해서 만들어진 아이 같은데,
post일 때 어떻게 동작할 지도 고민해보시면 좋을 것 같습니다!
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.
GET에서는 Query를 사용하여 데이터에 접근하지만 POST의 경우 Query를 사용하지 않고 대신 request body에 작성합니다. 때문에 Query가 없을 수도 있는 POST를 위해 타입을 옵셔널로 변경해 주었습니다.
관련 커밋 : 02c528f
private let boxOfficeDataCompletion: (Result<Data, NetworkError>) -> Void = { result in | ||
switch result { | ||
case .success(let data): | ||
guard let decodedData = BoxOfficeData.decode(data: data) else { | ||
return | ||
} | ||
case .failure(let error): | ||
print(error.localizedDescription) | ||
} | ||
} | ||
|
||
private let movieInformationCompletion: (Result<Data, NetworkError>) -> Void = { result in | ||
switch result { | ||
case .success(let data): | ||
guard let decodedData = MovieInformation.decode(data: data) else { | ||
return | ||
} | ||
case .failure(let error): | ||
print(error.localizedDescription) | ||
} | ||
} | ||
|
||
enum completion { | ||
case boxOfficeData | ||
case movieInformation | ||
|
||
var handler: (Result<Data, NetworkError>) -> Void { | ||
switch self { | ||
case .boxOfficeData: | ||
return NetworkManager().boxOfficeDataCompletion | ||
case .movieInformation: | ||
return NetworkManager().movieInformationCompletion | ||
} | ||
} | ||
} |
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.
요 부분은 제네릭을 고민해보시면 될 듯 해요!
다른 네트워크 구현체들 예제 찾아보시면 금방 답이 나올 듯 싶습니다.
} | ||
|
||
func fetchData(request: URLRequest, completion: @escaping (Result<Data, NetworkError>) -> Void) { | ||
let task = URLSession.shared.dataTask(with: request) { data, response, error in |
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을 엄청나게 깊게 파보지까지는 못해서 정확한 답변을 드리기 힘들 수 있지만,
제 생각에 지금 요구 조건에서는 말씀하신대로 shared를 써도 무방할 것 같아요.
하지만 저희가 목표하는 것은 취업이고, 취업을 준비하려면 대규모 슈퍼앱에서 네트워크 핸들링 하는 경우를 가정해야 할 것 같아요.
그랬을 때 shared를 쓰지 않고 configuration을 사용했을 때의 장/단점을 고민(구글링)해서 고민한 부분을 말씀해주시면 좋을 것 같습니다~
@havilog
안녕하십니까 하비~! 훈민트입니다.
어찌저찌 겨우겨우 STEP 2 PR 을 보냅니닿ㅎㅎ
잘 부탁드려요~!
고민했던 점
URLSession shared
사용URLSession 클래스가 task 생성을 위해 기본으로 제공하는 싱글톤 객체입니다.
shared
를 사용하는 경우 다음과 같은 제한이 있었습니다.위와 같은 제한 사항이 존재하기 때문에
shared
를 사용하여는 경우 일반적으로cache
,cookie storage
또는credential storage
를 커스텀 하여 사용하는 것을 피해야 합니다.현재 프로젝트를 진행하며 위와 같은 제한 사항과 관련한 내용이 없기 때문에
shared
를 사용하였습니다.추가적으로 찾은 자료에서는
shared
를 사용하는 경우 configuration을 사용하여 설정하는 부분에 대한 오버헤드가 없기 때문에 더 빠른 속도를 보인다는 내용이었습니다.해결하지 못한 점
handler
중복 합치기fetchData
메서드에escaping closure
를 사용해completion handler
를 구현하였습니다.Result
타입을 사용해 실패하면error
를 반환하고 성공하면data
를 반환하는데 그때 해당json data
를decode
해 사용할 수 있는 모델을 만들고 싶었습니다. 이때Decodable
을 채택한 2개의 서로 다른 모델 타입을 제외하고는completion handler
의 내용이 같기에 하나의 프로퍼티로 선언해 전달해주고 싶었습니다.위의 코드에서 guard let 안의 BoxOfficeData만 MovieInformation과 옵션을 줘서 번갈아가며 사용할 수 있게 해주고 싶었습니다.
위와 같은 이유로 다음과 같이 2가지 방법을 생각해 보았지만 두 방법 모두 성공하진 못했습니다.
closure로 타입 전달
클로저로 타입을 전달해주는 방법입니다. 다만 이 경우 fetchData 함수에서도 성공한 경우 completion(.success((
type
, data)))로 fetchData에서부터 type을 전달해주어야 합니다. 그러나 이때 type을 전달해 줄 수가 없었습니다.fetchData 함수를 부르는 함수에 타입 전달
그러나 이렇게 한 후 사용이 가능한지 실험해보는 과정에서 다음과 같은 에러가 발생하며 Decodable 타입을 사용할 수가 없었습니다.
결국 합치지는 못하고 각각 다른 completion handler를 만들어 enum을 통해 전해줄 수 있게 구현하였습니다.
조언을 얻고 싶은 점
URLSession shared
사용 예시shared
를 사용하는 것이 좋은지를 찾지 못하였습니다. 기본적으로URLSession.init(configuration: .default)
를 바탕으로shared
가 만들어졌다고 하는데 어떤 경우에shared
를 사용할 수 있을까요?default
와shared
를 사용하는 기준점이 위의 제한 사항 외에도 다른 이유가 있을까요?