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 2] hoon, MINT #110

Draft
wants to merge 13 commits into
base: ic_9_mint09
Choose a base branch
from

Conversation

mint3382
Copy link

@havilog
안녕하십니까 하비~! 훈민트입니다.
어찌저찌 겨우겨우 STEP 2 PR 을 보냅니닿ㅎㅎ
잘 부탁드려요~!

고민했던 점

URLSession shared 사용

  • URLSession 클래스가 task 생성을 위해 기본으로 제공하는 싱글톤 객체입니다. shared를 사용하는 경우 다음과 같은 제한이 있었습니다.

    • 데이터가 서버에서 도착할 때 점진적으로 데이터를 얻을 수 없다.
    • 기본 연결 동작을 사용자 지정할 수 없다.
    • 인증 기능에 제한이 있다.
    • 앱이 실행 중이지 않은 상태에서 다운로드나 업로드를 실행할 수 없다.

    위와 같은 제한 사항이 존재하기 때문에 shared를 사용하여는 경우 일반적으로 cache, cookie storage 또는 credential storage를 커스텀 하여 사용하는 것을 피해야 합니다.

    현재 프로젝트를 진행하며 위와 같은 제한 사항과 관련한 내용이 없기 때문에 shared를 사용하였습니다.

    let task = URLSession.shared.dataTask(with: request) { data, response, error in
        if error != nil {
            completion(.failure(.dataTaskFail))
            return
        }
    
        guard let httpResponse = response as? HTTPURLResponse else {
            completion(.failure(.responseCasting))
            return
        }
    
        guard (200...299).contains(httpResponse.statusCode) else {
            completion(.failure(.invalidStatus))
            return
        }
    
        guard let data = data else {
            completion(.failure(.noData))
            return
        }
    
        completion(.success(data))
    }
    task.resume()

    추가적으로 찾은 자료에서는 shared를 사용하는 경우 configuration을 사용하여 설정하는 부분에 대한 오버헤드가 없기 때문에 더 빠른 속도를 보인다는 내용이었습니다.

해결하지 못한 점

handler 중복 합치기

  • fetchData 메서드에 escaping closure를 사용해 completion handler를 구현하였습니다. Result 타입을 사용해 실패하면 error를 반환하고 성공하면 data를 반환하는데 그때 해당 json datadecode해 사용할 수 있는 모델을 만들고 싶었습니다. 이때 Decodable을 채택한 2개의 서로 다른 모델 타입을 제외하고는 completion handler의 내용이 같기에 하나의 프로퍼티로 선언해 전달해주고 싶었습니다.

    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)
        }
    }

    위의 코드에서 guard let 안의 BoxOfficeData만 MovieInformation과 옵션을 줘서 번갈아가며 사용할 수 있게 해주고 싶었습니다.

  • 위와 같은 이유로 다음과 같이 2가지 방법을 생각해 보았지만 두 방법 모두 성공하진 못했습니다.

    1. closure로 타입 전달

      private let boxOfficeDataCompletion: (Decodable, (Result<Data, NetworkError>)) -> Void = { type, result in
          switch result {
          case .success(let data):
              guard let decodedData = type.decode(data: data) else {
                  return
              }
          case .failure(let error):
              print(error.localizedDescription)
          }
      }

      클로저로 타입을 전달해주는 방법입니다. 다만 이 경우 fetchData 함수에서도 성공한 경우 completion(.success((type, data)))로 fetchData에서부터 type을 전달해주어야 합니다. 그러나 이때 type을 전달해 줄 수가 없었습니다.

    2. fetchData 함수를 부르는 함수에 타입 전달

      func prepareData<T: Decodable>(request: URLRequest, type: T) {
          let handler: (Result<Data, NetworkError>) -> Void = { result in
              switch result {
              case .success(let data):
                  guard let decodedData = type.decode(data: data) else {
                      return
                  }
                  print(decodedData)
              case .failure(let error):
                  print(error.localizedDescription)
              }
          }
          fetchData(request: request, completion: handler)
      }

      그러나 이렇게 한 후 사용이 가능한지 실험해보는 과정에서 다음과 같은 에러가 발생하며 Decodable 타입을 사용할 수가 없었습니다.

  • 결국 합치지는 못하고 각각 다른 completion handler를 만들어 enum을 통해 전해줄 수 있게 구현하였습니다.

    enum completion {
        case boxOfficeData
        case movieInformation
        
        var handler: (Result<Data, NetworkError>) -> Void {
            switch self {
            case .boxOfficeData:
                return NetworkManager().boxOfficeDataCompletion
            case .movieInformation:
                return NetworkManager().movieInformationCompletion
            }
        }
    }

조언을 얻고 싶은 점

URLSession shared 사용 예시

  • 어떤 예시에서 shared를 사용하는 것이 좋은지를 찾지 못하였습니다. 기본적으로 URLSession.init(configuration: .default)를 바탕으로 shared가 만들어졌다고 하는데 어떤 경우에 shared를 사용할 수 있을까요? defaultshared를 사용하는 기준점이 위의 제한 사항 외에도 다른 이유가 있을까요?

Copy link

@havilog havilog left a comment

Choose a reason for hiding this comment

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

수고하셨어요 훈트!

전체적인 일정을 잡고, 네트워크의 본질적인 부분에 대해 고민하느라 앞쪽에서 시간을 많이 소비해서 코드적으로 다른 캠퍼들에 비해 많이 못봐드려서 아쉽네요 ㅠㅠ

하지만 절대 무의미한 시간은 아니었을거라고 생각되어요!

다른 네트워크 구현체들 보고 참고해서

전체적으로 어떤 부분을 놓치고 있었는지,
확장성 있는 네트워크가 되려면 어떻게 해야할 지

고민해보시면 좋을 듯 싶습니다!

.DS_Store Outdated
Copy link

Choose a reason for hiding this comment

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

dsstore가 올라와 있네요!

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 10 to 22
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
}
}
Copy link

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 {
Copy link

Choose a reason for hiding this comment

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

Suggested change
enum CodingKeys: String, CodingKey {
private enum CodingKeys: String, CodingKey {

Copy link

Choose a reason for hiding this comment

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

접근 제어를 사용하여 외부에서 접근하지 못하도록 수정하였습니다.

관련 커밋: 8c8af63

Comment on lines 15 to 28
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"
}
}
Copy link

Choose a reason for hiding this comment

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

HTTPMethod의 rawValue를 안쓰고 typeName을 정의해주신 이유가 있을까요?

Copy link
Author

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 {
Copy link

Choose a reason for hiding this comment

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

NetworkManager를 struct로 구현하신 이유에 대해 설명해주시면 좋을 것 같아요!

Comment on lines 58 to 60
guard let url = component.url else {
fatalError(NetworkError.invalidURL.localizedDescription)
}
Copy link

Choose a reason for hiding this comment

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

fatalError를 핸들링 할 수 있으면 좋을거 같아요

Copy link
Author

Choose a reason for hiding this comment

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

fetalError를 핸들링한다는 말이 혹시 do-catch가 될 수 있게 바꾸는 것을 말씀 하시는 걸까요? 아니면 사용자에게 보이도록 알람과 같은 기능을 띄우는 것을 의미하시는 걸까요?

Comment on lines 24 to 34
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)
}
Copy link

Choose a reason for hiding this comment

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

공통적으로 쓰이는 key값이 있는건 괜찮을 거 같지만,
만약

  1. 로그인하는 api
  2. 유저 정보를 가져오는 api
  3. 영화 정보를 가져오는 api

등이 있을 때 이 Query가 어떻게 확장될 수 있을지 고민해보시면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

여러 주소에서 사용할 수 있도록 URLQuery를 protocol로 구현하였습니다.
관련 커밋: bed67b1

}
}

private func configureURL(path: String, query: [URLQueryItem?]) -> URL {
Copy link

Choose a reason for hiding this comment

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

요 아이도 현재는 get만 고려해서 만들어진 아이 같은데,
post일 때 어떻게 동작할 지도 고민해보시면 좋을 것 같습니다!

Copy link
Author

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

Comment on lines 11 to 45
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
}
}
}
Copy link

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
Copy link

Choose a reason for hiding this comment

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

저도 뭔가 URLSession을 엄청나게 깊게 파보지까지는 못해서 정확한 답변을 드리기 힘들 수 있지만,

제 생각에 지금 요구 조건에서는 말씀하신대로 shared를 써도 무방할 것 같아요.

하지만 저희가 목표하는 것은 취업이고, 취업을 준비하려면 대규모 슈퍼앱에서 네트워크 핸들링 하는 경우를 가정해야 할 것 같아요.

그랬을 때 shared를 쓰지 않고 configuration을 사용했을 때의 장/단점을 고민(구글링)해서 고민한 부분을 말씀해주시면 좋을 것 같습니다~

@mint3382 mint3382 marked this pull request as draft August 18, 2023 02:12
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.

None yet

3 participants