Conversation
jaemuYeo
left a comment
jaemuYeo
left a comment
There was a problem hiding this comment.
씨유 루키 송만이 고생많았습니다!!
코드에 코멘트 남겨놓을게요!!
커밋에 대해서 조금 더 세분화시키지 못한부분이 너무 아쉽게 느껴집니다 😭
조금 더 나누어서 커밋 기록을 남겨보면 좋을 것같아요!!
코드의 구현이 잘 동작하는지 네이밍은 틀리지 않았는지 이런 코드에 대한 부분도 중요하지만
그 외에 프로젝트를 진행하는 동안의 커뮤니케이션과 커밋 이력 PR 등도
아주 중요한 요소입니다 🙂👍
|
|
||
| import UIKit | ||
|
|
||
| struct JSONParser<T: Decodable> { |
There was a problem hiding this comment.
오우!! JSON을 파싱해주는 타입을 만들어보았군요!! 👍
혹시 구조체로 하지않고 POP를 활용하는 방법은 어떻게 생각하시나요??
| import UIKit | ||
|
|
||
| struct JSONParser<T: Decodable> { | ||
| enum DecodeType { |
There was a problem hiding this comment.
삭제해야될 부분인데 삭제하지 않았는거 같습니다. 수정하겠습니다.
| case string | ||
| } | ||
| func decode(data: Data?) throws -> T { | ||
| guard let data = data, let decodingData = try? JSONDecoder().decode(T.self, from: data) else { |
|
|
||
| import Foundation | ||
|
|
||
| enum MyError: Error, LocalizedError { |
There was a problem hiding this comment.
LocalizedError와 errorDescription의 활용 너무 좋습니다 💯
다만 MyError이라는 네이밍이 와닿지 않아요~~
에러 타입의 네이밍을 정할때는 어떤 것에 대한 에러인지 명확하게 해주면 좋습니다!!
ex) APIError, OpenMarketError, DecodingError ,,,
There was a problem hiding this comment.
JSON에 대한 오류와 네트워크에 관한 오류들을 잘 나타낼수 있도록 네이밍 수정해보겠습니다!
| switch self { | ||
| case .unvalidURL: | ||
| return "url 생성 실패" | ||
| case .parsingError: |
There was a problem hiding this comment.
해당 케이스의 두 에러는 서로 다른 주최가 있는걸로 생각해요
위에 네이밍에 전달한것 처럼 API에 대한 에러, 파싱에 대한 에러 등등 여러 분류로 나뉠수 있을 것 같은데
에러 타입을 분리해보는 것은 어떨까요??
| import UIKit | ||
|
|
||
| struct SessionData<T: Decodable> { | ||
| private let host: String = "https://openmarket.yagom-academy.kr/" |
There was a problem hiding this comment.
host라는 간단한 네이밍보다 조금 더 명확했으면 좋겠어요!
| URL(string: "\(host)\(url.description)") | ||
| } | ||
|
|
||
| func getSessionData(from dataDescription: URLType) throws -> T { |
There was a problem hiding this comment.
해당 메서드내부에 Response에 대한 부분과 error가 발생했을때에 처리는 해주지 않았네요!
그 부분을 빼신 이유가 있나요??
|
|
||
| func getSessionData(from dataDescription: URLType) throws -> T { | ||
| var resultData: Data? | ||
| let semaphore = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
세마포어의 기능은 무엇인가요??
해당 메서드에 세마포어를 통해 구현한 이유가 궁금해요 🤔
|
|
||
| import Foundation | ||
|
|
||
| enum URLType { |
There was a problem hiding this comment.
이 곳에 들어갈 것들은 URL의 구성중 어떤 부분인가요?
예를들어 host, query parameter, path 등에서 어떤것들의 모음인지 정한 후
거기에 맞게 네이밍해주어도 좋을 것같습니다!
There was a problem hiding this comment.
어떤 데이터를 불러올지 결정하기 위한 열거체인데 query parameter와 path 등을 한꺼번에 다루기 어려워 네이밍을 수정함과 동시에 분리할 수 있도록 해보겠습니다!
| import XCTest | ||
| @testable import OpenMarket | ||
|
|
||
| class OpenMarketDataParsingTest: XCTestCase { |
There was a problem hiding this comment.
Test는 실제 API가 아닌 Mock 객체를 활용해주세요~
현재 테스트케이스들이 실행이 되고 있지않습니다!
에러에 대해 확인해보세요 🙂
수정할때에 given when then 패턴을 활용해보면 좋겠습니다!!
4 participants
@jaemuYeo
여러모로 부족한 코드입니다..
작업 내용
Page
Product
MyError
URLType
실행 화면
트러블 슈팅
1-1. 실제로 api서버에서 받아오는 데이터와 사전에 받은 Mock 데이터 중 일부 인스턴스의 이름이 다른 점이 있었던 것으로 확인되어 수정 후 문제를 해결