박스오피스 앱 [STEP 4] Matthew, Kyle #57
Conversation
Step4 temp
There was a problem hiding this comment.
안녕하세요 카일, 매튜
이번 스텝도 고생 많으셨습니다.
코드에 관련된 것들은 코멘트로 남겨두었습니다.
고민한점이나 질문 주신 것들에 대해서는 전체 코멘트로 남겨두겠습니다.
[completion handler → async-await]
제가 생각했을때는 completion을 사용해서 발생한 문제는 아니었다고 생각해요. 만약 그런 문제라면 async-await이 나오기 전에는 이러한 요청을 하기 못하거나 굉장히 어려웠다는 얘기가 될테니까요.
먼저 궁금한 부분은 요청이 여러번 된다는 것인지, 요청이 예상했던 순서대로 되지 않았던 것인지 등이 궁금해요.
그리고 다른 질문 내용이나 고민한 점에도 공통으로 드리고 싶은 얘기인데 여러분들이 겪은 문제가 왜 발생했는지를 먼저 파악하는 것이 중요해요.
제일 먼저 지금 우리의 코드가 어떻게 동작하고 있고 왜 이렇게 동작하는지를 파악하는게 중요해요.
원인을 파악하지 않고 계속 새로운 방법을 찾아서 해결을 하려고 하면 나중에 비슷한 문제가 나오더라도 해결하기 어려울거에요.
꼭 원인을 구체적으로 파악하고 해결하셨으면 좋을 것 같아요.
그리고 지금의 async-await을 선택하셨는데 기존에 어떤 문제가 있었고 async-await을 사용하면 왜 문제가 해결되었는지도 구체적으로 알아보시면 좋을 것 같아요.
[dataTask(with: url) → dataTask(with: request)]
이 부분은 어떤 의도로 말씀하셨고 왜 이렇게 리팩토링하신건지 조금 이해하기 어려운 것 같네요.
조금 더 구체적으로 알려주시면 감사하겠습니다~
[ATS Policy]
현재 구현 하신 방법으로도 해결할 수도 있겠지만 강제로 변경하고 합치고 있는 것 같긴해요.
url형태의 문자열에서 https위치를 찾아 https -> http로 단순 문자 교체를 하고 있는 형태니까요.
먼저 어떤 에러를 겪으셨는지는 모르겠지만 제목인 ATS Policy로 검색해도 많은 것들이 나오니 한번 확인해보시면 좋을 것 같습니다.
그리고 비슷하지만 조금 더 URL스럽게 https -> http로 바꿀 수 있는 방법은 여러분들의 코드에서 사용한 것을 활용해도 가능합니다.
힌트는 NetworkURL파일에 있고 그 코드가 어떻게 동작하고 있는지 알아보시면 개선하는데 도움이 될 것 같네요!
[ReusedDetailStackView]
동일한 UI가 반복되는 경우에는 실제로 하나의 UI를 구현해두고 재사용하는 방법을 사용하기도 합니다.
어떤 이유때문에 이 방법에 대해 의심을 가지게 되었는지가 궁금하네요!
[URLRequest 생성 로직]
지금의 형태도 괜찮은 것 같습니다. 말씀해주신 단점은 조금 주관적인 느낌이라 조금 더 구체적으로 생각하신 것들을 알려주시면 좋을 것 같네요.
저는 현재의 형태도 크게 문제가 있어보이진 않아요 ㅎㅎ
하나 더 욕심을 내자면 지금은 "GET"요청만 하고 있지만 "POST", "PATCH"등의 요청을 해야하는 경우에는 대응이 안될 수 있을 것 같네요.
물론 이번 프로젝트에서는 없을 것 같아 필수로 수정하실 필요는 없습니다!
현재 코드를 조금 다른 방법으로 풀어보자면 URLRequest에 필요한 host, path, query, header, httpMethod를 하나로 묶어서 case로 만들어서 파라미터로 하나만 받아서 request를 만드는 방법도 있을 것 같네요.
그럼 현재 카카오, 영화진흥원 으로 나뉘고 차트, 상세, 이미지로 나뉘어 있는 것을 하나로 묶어서 관리할 수 있지 않을까요?
P.S 의문이 들거나 현재의 방법이 옳지 않은 것 같은 의심이 든다면 왜 그런 생각이 들었는지를 구체적으로 알려주시면 저도 그에 맞게 더 구체적으로 답변을 드릴 수 있을 것 같아요.
| private let movieName: String | ||
| private let movieCode: String | ||
| private let movieManager: MovieManager | ||
| private let scrollView = BoxOfficeDetailView() |
There was a problem hiding this comment.
BoxOfficeDetailView가 ScrollView를 상속받고 있긴 하지만 하는 역할은 영화 상세 페이지에 대한 UI가 담겨 있는 것 같습니다.
그에 맞게 네이밍이 수정되면 좋을 것 같아요.
There was a problem hiding this comment.
넵! boxOfficeDetaiView로 수정하여 반영하도록 하겠습니다~!
| func setupBoxOfficeListView() { | ||
| LoadingIndicatorView.showLoading() | ||
| boxOfficeListView.boxOfficeListDelegate = self | ||
| boxOfficeListView.delegate = self | ||
| boxOfficeListView.dataSource = dataSource | ||
| view = boxOfficeListView | ||
| } |
There was a problem hiding this comment.
비슷한 역할을 하는 것들끼리 묶어두셨네요! 좋은 방법인 것 같습니다.
|
|
||
| import Foundation | ||
|
|
||
| enum APIQuery { |
There was a problem hiding this comment.
쿼리에서 header까지 만들고 있는게 조금 어색한 것 같아요.
쿼리에 대해 한번 더 알아보시면 좋을 것 같아요.
그리고 현재 구현된 것을 봤을때는 카카오나 영화진흥원 API요청을 하기 위한 데이터들을 연산프로퍼티로 구현해두신 것 같은데 APIType 내부에 같이 구현해둬도 괜찮을 것 같긴하네요
There was a problem hiding this comment.
아래와 같이 수정하여 반영했습니다! 로직이 훨씬 깔끔해졌네요ㅎㅎ
enum APIType {
case kakao(movieName: String)
case boxOffice(BoxOfficeType)
var host: String {
switch self {
case .boxOffice:
return "www.kobis.or.kr"
case .kakao:
return "dapi.kakao.com"
}
}
var header: [String:String]? {
switch self {
case .kakao:
["Authorization": "KakaoAK \(Bundle.main.kakaoApiKey)"]
case .boxOffice:
nil
}
}
var queries: [URLQueryItem] {
switch self {
case .kakao(let movieName):
[
URLQueryItem(name: "query", value: "\(movieName) 영화포스터"),
URLQueryItem(name: "sort", value: "accuracy")
]
case .boxOffice(let type):
type.queries
}
}
var path: String {
switch self {
case .kakao(_):
"/v2/search/image"
case .boxOffice(let type):
type.path
}
}
}|
|
||
| import UIKit | ||
|
|
||
| final class BoxOfficeDetailView: UIScrollView { |
There was a problem hiding this comment.
UIView에 ScrollView를 넣어서 구현하지 않고 ScrollView 자체를 상속받으신 이유가 궁금합니다!
이 부분은 정말 궁금해서 여쭤보는거에요!
There was a problem hiding this comment.
현재 명세서에서 스크롤뷰 외에 부가적인 UI뷰 요소가 없어서 DetailView 자체를 scrollView로 생성했습니다!
|
|
||
| import UIKit | ||
|
|
||
| struct LoadingIndicatorView { |
There was a problem hiding this comment.
구조체 타입으로 static하게 선언이라고 하셨는데 사실 static하게 선언하는 것은 struct, enum, class 모두 가능합니다.
두가지 개념이 묶여 있지 않아요.
다른 사람들은 어떻게 구현하는지 확인해보면서 왜 그렇게 구현했을까도 고민해보면 좋겠네요.
There was a problem hiding this comment.
struct타입이라 static하게 선언이 구조체라서 static이 아닌 그냥 구조체로 변경하고 안에 있는 요소를 static으로 선언한 것 입니다!
struct타입이라 static하게 선언에는 오해가 있었네요 ㅜ
struct를 사용한 이유는 상속을 하지않을 것이라 판단하여 사용했습니다.
코멘트 달아주신 부분을 찾아보니 현재 indicatorView가 nameSpace 처럼 사용되고 있어서 인스턴스화 하지 않기에 struct보단 enum을 활용하는게 더 올바른것 같습니다!
enum으로 수정해보도록 하겠습니다!
|
|
||
| import UIKit | ||
|
|
||
| struct LoadingIndicatorView { |
There was a problem hiding this comment.
[Bonus]
전역으로 로딩을 띄우고 내리는 것을 직접 구현하신 것은 정말 잘 하신 것 같습니다.
추가로 보너스 스텝의 느낌으로 코멘트를 남기자면 지금은 showLoading에서는 로딩이 띄워져있지 않으면 띄운다, hideLoading에서는 로딩이 띄워져있으면 내린다로 되어있는데 show를 두번하고 hide를 한번 내린다면 어떻게 될지 생각해보시고 어떻게 예방할 수 있을지 고민해보시면 좋을 것 같아요.
위의 같은 상황을 조금 구체적으로 예를 들면 로딩이 필요한 요청을 2번 했는데 하나가 먼저 끝나고 하나는 아직 요청을 불러오는 중이 될 수 있겠죠?
There was a problem hiding this comment.
window가 아닌 view를 파라미터로 받아서 해당 view의 요청이 끝날때 독립적으로 hide 되도록 하였습니다!
추가적으로 기존에 rootViewController에서 indicator가 navigation영역만큼 내려가는 이슈가 발생했습니다.
따라서 디버깅 결과
indicator.bounds를 통해 y값이 navigation 영역만큼 - 되는걸 발견하여
indicator.center의 y를 view.frame.height / 2 + view.bounds.origin.y 더해서 center로 맞췄습니다!
loadingIndicatorView = UIActivityIndicatorView()
loadingIndicatorView.frame = view.frame
loadingIndicatorView.center = CGPoint(
x: view.frame.width / 2,
y: view.frame.height / 2 + view.bounds.origin.y
)
view.addSubview(loadingIndicatorView)
loadingIndicatorView.startAnimating()
카카오 검색 // 수정 전
// APIService
func fetchImageData<T: Decodable>(urlRequest: URLRequest, completion: @escaping (Result<T, NetworkError>) -> Void) {
self.session.dataTask(with: urlRequest) { data, response, error in
DispatchQueue.global().async {
self.handleDataTaskCompletion(
data: data,
response: response,
error: error,
completion: completion
)
}
}.resume()
}
func fetchData<T: Decodable>(urlString: String, completion: @escaping (Result<T, NetworkError>) -> Void) {
/*{ code }*/
self.session.dataTask(with: url) { data, response, error in
DispatchQueue.global().async {
self.handleDataTaskCompletion(
data: data,
response: response,
error: error,
completion: completion
)
}
}.resume()
}
// 수정 후
// APIService
func fetchData(with urlRequest: URLRequest) async throws -> APIResult {
guard
let cachedResponse = URLCache.shared.cachedResponse(for: urlRequest)
else {
let (data, response) = try await self.session.requestData(with: urlRequest)
let cachedURLResponse = CachedURLResponse(response: response, data: data)
URLCache.shared.storeCachedResponse(cachedURLResponse, for: urlRequest)
return handleDataTaskCompletion(data: data, response: response)
}
return handleDataTaskCompletion(data: cachedResponse.data, response: cachedResponse.response)
}
저희가 해당 문제를 인지하고 enum NetworkURL {
static func makeURLRequest(type: APIType, httpMethod: HttpMethod = .get) -> URLRequest? {
let urlComponents = makeURLComponents(type: type)
guard
let url = urlComponents.url
else {
return nil
}
var request = URLRequest(url: url)
request.httpMethod = httpMethod.type
guard
let header = type.header
else {
return request
}
request.allHTTPHeaderFields = header
return request
}
private static func makeURLComponents(type: APIType) -> URLComponents {
var urlComponents = URLComponents()
urlComponents.scheme = "https"
urlComponents.host = type.host
urlComponents.path = type.path
urlComponents.queryItems = type.queries
return urlComponents
}
}
현재 해당하는 로직이 각 컴포넌트 하나씩
주말에 매튜와 만나서 정말 열심히 고민한 부분인데요..! 아래와 같이 리팩토링 해보았습니다! enum APIType {
case kakao(movieName: String)
case boxOffice(BoxOfficeType)
case image(url: URL)
var host: String? {
/*{code}*/
}
var header: [String:String]? {
/*{code}*/
}
var queries: [URLQueryItem]? {
/*{code}*/
}
var path: String {
/*{code}*/
}
}
enum HttpMethod {
case get
case post
case put
case patch
case delete
var type: String {
/*{code}*/
}
}
// NetworkURL
static func makeURLRequest(type: APIType, httpMethod: HttpMethod = .get) -> URLRequest? {
/*{code}*/
var request = URLRequest(url: url)
request.httpMethod = httpMethod.type
/*{code}*/
return request
} |
| Task { | ||
| do { | ||
| let result = try await movieManager.fetchBoxOfficeResultData(date: Date.movieDateToString) | ||
| self.reloadCollectionListData(result: result) | ||
| LoadingIndicatorView.hideLoading(in: self.view) | ||
| } catch { | ||
| print(error.localizedDescription) | ||
| } |
There was a problem hiding this comment.
로딩 인디케이터가 사라져야 하는 시점이 언제일지 다시 한번 확인해보세요~
| Task { | ||
| do { | ||
| try await fetchMoiveImageURL() | ||
| try await fetchMovieInfo() | ||
| try await setupMovieImage() | ||
| } catch { | ||
| print(error.localizedDescription) | ||
| } | ||
| LoadingIndicatorView.hideLoading(in: self.boxOfficeDetailView) | ||
| } |
There was a problem hiding this comment.
여기도 로딩 인디케이터가 사라져야 하는 시점이 언제일지 다시 한번 확인해보세요~
박스오피스 앱 [STEP 4]
멤버 : @kimbs5899 @Changhyun-Kyle
리뷰어 : @ICS-Asan
박스오피스앱 네번째 Step입니다!
정리하여 PR 드립니다. 감사합니다!
이번 스텝을 통해서 배운 점
Swift Concurreny: async-awaitUIScrollViewUIApplication / WindowSceneURLCache🤔 고민한 점
1.
completion handler→async-await이번
STEP에서Detail View에서fetch받아야할 데이터는 3가지였습니다.movieInfo데이터 받아오기movieName으로 이미지 url 받아오기url로 이미지 불어오기하지만, 기존
completion handler를 활용한다면 다중콜백이 발생하였습니다. 따라서, 데이터 통신 구현을async-await로 리팩토링하였습니다.2.
dataTask(with: url)→dataTask(with: request)이번
STEP에서 추가된 카카오 이미지 검색 API는HTTP Header가 필수로 필요하여URLRequest를 통해 데이터 통신을 구현해야했습니다. 이렇게 된다면url통신과request통신을 따로 구현해야하기 때문에URLRequest를 통해 데이터 통신을 할 수 있게 리팩토링했습니다.3.
MovieRepository레이어 추가STEP에서 추가된 네트워크 통신 로직을 구현하면서 현재 설계에 대해 고민해보았습니다.APIService: 네트워크 통신을 수행하고 JSON 데이터를 디코딩MovieManager:APIService에서 디코딩된 데이터를 각 모델 타입에 맞게 변경 후 비즈니스 로직 수행JSON디코딩이 필요하지 않아 이에 해당하는 로직을APIService에서 새로 구현해야하는 문제점을 발견할 수 있었습니다.APIService: 네트워크 통신을 수행합니다.MovieRepository:APIService에서 받은 데이터로 필요한 모델 타입으로 변환(디코딩)합니다.MovieManager:MovieRepository에서 변환된 데이터로Controller에서 필요한 비즈니스 로직을 수행합니다.4.
Progressive LoadingvsLoading Indicator네트워크 통신을 통해 데이터를 패치하는 동안
UI를 어떻게 구현할 지 고민했습니다.처음에는
URLSessionDataDelegate를 활용하여 이미지를progressive하게 로드하여 보여주려고 했으나 해당 프로토콜을 활용하기 위해선dataTask(with:)활용 시completion hanedler를 사용할 수 없고 이미지가jpeg인 경우에만 활용할 수 있어 제약이 있었습니다.따라서, 이미지에
placeholder를 지정하고UIActivityIndicatorView를 활용했습니다. 처음 앱이 실행되었을 때 또한UIActivityIndicatorView가 활용되기 때문에 구조체 타입으로static하게 선언하여 매번 뷰 최상단에 설정할 필요없이windowScene에 접근하여 보여줄 수 있도록 구현했습니다.5.
URLCache현재 네트워크 통신을 하는 부분이 상당히 많아
Cache도입에 대해 고민해보았습니다.앞서 진행한 리팩토링으로
URLRequest를 활용할 수 있게 되어URLCache를 사용하여cacheResponse가 있다면 네트워크 통신을 하지 않고 해당response로 데이터를 패치할 수 있도록 구현했습니다.🧐 궁금한 점
1.
ATS Policy이번
STEP명세서에서 이미지url을 패치할 때, 카카오 이미지 검색 결과의 첫번째 이미지url를 받아오게 구현해야 했습니다. 이때, 이미지url의scheme이https가 아닌http인 경우ATS Policy를 위반하기 때문에 이미지가 불러와지지 않는 이슈가 발생했습니다. 이에 따라, 이미지url의scheme을https로 바꿔주는 로직을 추가했는데 과연 올바른 대응인지 궁금합니다.2. ReusedDetailStackView
이번
STEP에서Label:Label과 같은 형태로 생성한 스택뷰를 매번 생성하지 않고 따로ReusedDetailStackView로 생성하여 사용했습니다!이렇게 생성하여 데이터를 주입하는 방식을 현재 로직에 적용했는데 보다 조금더 효율적 방식이 있는지 궁금합니다!
3.
URLRequest생성 로직URLComponents를 활용하여URLRequest를 생성하는 로직에서 중복되는 코드들이 많아enum을 활용하여 일반화 했습니다. 하지만, 이에 대한 단점으로 너무nested하고 오히려 복잡하게 되지 않았나하는 생각이 드는데 아샌의 의견이 궁금합니다!