-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] SNMNetwork 구현 #51
Conversation
- NetworkProvider 프로토콜 구현 - SNMNetworkProvider 타입 구현
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.
수고하셨습니당!! 혜민님:)
do { | ||
let jsonEncoder: JSONEncoder = JSONEncoder() | ||
let encodedData = try jsonEncoder.encode(body) | ||
urlRequest.httpBody = encodedData |
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.
P3. 메서드가 실행될 때마다 새로운 JSONEncoder 객체를 생성되네요.
따로 타입을 분리하고 static 프로퍼티로 보관하여 사용한다면 생성 횟수를 줄일 수 있지 않을까 생각합니당
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.
맞습니다! 쓸데없는 리소스 낭비네요... 사실 이 친구, 어떻게 분리해야할지 감이 안잡혔는데 말씀해주신대로 AnyEncodable 타입으로 분리하고 static 프로퍼티로 jsonEncoder를 관리하면 좋겠네요!
감사합니다~~ 개선해볼게요
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.
정말 수고하셨습니다 👍👍👍
public struct Endpoint { | ||
public var baseURL: URL | ||
public var path: String | ||
public var method: HTTPMethod | ||
public var query: [String: String]? | ||
/// path와 query를 조합한 URL입니다. 조합에 실패시 baseURL을 반환합니다. | ||
public var absoluteURL: URL { | ||
var absoluteURLString: String = baseURL.absoluteString + "/" | ||
// 의미없는 / 제거 | ||
let pathComponents = path.components(separatedBy: "/") | ||
.compactMap{ $0.isEmpty ? nil : $0 } | ||
let queryPathString: String = query?.map{ "\($0)=\($1)" }.joined(separator: "&") ?? "" | ||
absoluteURLString += pathComponents.joined(separator: "/") | ||
if !queryPathString.isEmpty { | ||
absoluteURLString += "?\(queryPathString)" | ||
} | ||
|
||
return URL(string: absoluteURLString) ?? baseURL | ||
} | ||
} |
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.
P5. URL Components를 사용해도 좋을거 같아요.
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.
오 URLComponents는 처음 알았는데, Multipart 적용하면서 같이 적용해보겠습니다~~ 감사해요!
// HTTP Method가 GET이면서 body가 존재하면 invalid로 처리 | ||
guard !(endpoint.method == .get) | ||
else { throw SNMNetworkError.invalidRequest(request: self) } |
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.
👍
🔖 Issue Number
close #13
📙 작업 내역
설계했던 NW 구조에 따라 구현을 진행했습니다.
SNMRequestConvertible의 extension을 통해 urlRequest로 변환하는 메서드를 선언해 실제 NetworkProviderd에서 URLSession 작업이 가능하도록 했습니다.
RequestConvertible를 파라미터로 받는 객체입니다.
request를 처리하는 과정에서 Error가 발생하면 SNMError로 무조건 매핑해 던집니다.
📝 PR 특이사항
편의를 위해 특정 HTTP statusCode와 관련된 에러 일부를 매핑해 내려주는 방식으로 구현을 진행했는데요.
에러가 많아서 다 컨트롤할 수 없거니와,,, 네트워크 모듈단에서 statusCode 에러까지 일일이 처리해주는 게 과한 것 같다는 생각이 들었습니다. 추후 개선할 예정입니다.
추후 모듈화 진행을 위해 모듈화 대상 객체에 public 키워드를 붙였습니다.