오픈 마켓 [STEP 1] 아리,제리

[leeari95]

안녕하세요. 그린 @GREENOVER

아리 @leeari95, 제리 @llghdud921 입니다!
이번 오픈마켓 프로젝트 리뷰어로 만나뵙게되어 영광입니다. 🙇🏻‍♀️

오픈마켓 STEP 1 너무 어려웠는데... 저희가 잘 구현했는지 모르겠네요. 🥲
저희가 아래 적은 내용 이외에 부족한 점이나 놓친 부분이 있다면 번거롭더라도 한번 더 짚어주시면 감사하겠습니다. 🙏🏻

고민했던 점

1. 단일 책임 원칙(Single responsibility principle)

• 한 타입이 하나의 역할만 할 수 있도록 설계에 많은 고민을 해보았습니다.

2. CodingKeys 활용

실제 네트워크에서 내려오는 변수명이 스네이크 케이스를 사용하는 변수는 Codingkey를 이용하여 parsing하는 key를 바꿔주었으며, 스네이크케이스를 사용하지 않는, 즉 타입의 변수명과 일치하면 rawValue를 명시할 필요가 없어 가독성을 위해 한 줄로 case를 합쳐주었습니다.

enum CodingKeys: String, CodingKey {
   case id, stock, name, thumbnail, currency, price, images, vendors
   case vendorID = "vendor_id"
   case bargainPrice = "bargain_price"
   case discountedPrice = "discounted_price"
   case createdAt = "created_at"
   case issuedAt = "issued_at"
}

3. NetworkManager와 Network

• Network하는 과정에서 역할마다 객체를 구분하여 구현하였습니다.

  • Network : dataTask()를 통해 SessionDataTask를 서버로 전송해 직접 네트워킹하는 객체

    func execute(request: URLRequest, completion: @escaping (Result<Data?, Error>) -> Void) {
            session.dataTask(with: request) { data, response, error in
            ...

  • NetworkManager : Network의 excute를 통해 data를 받아 decoding하는 fetch()를 가진 객체

    func fetch<T: Decodable>(request: URLRequest,
                                decodingType: T.Type,
                                completion: @escaping (Result<T, Error>) -> Void) {
            
         network.execute(request: request) { result in

4. Name Space

• 하드코딩을 개선하기 위해 enum 타입을 만들어 Address와 HTTPMethod의 값들을 분류해주었습니다.

5. Request, Response

• Request할 때, 그리고 Response하는 타입이 세부적으로 달라 ProductModification, ProductRegistration 등... 각 타입을 모두 구현하였습니다.

6. Overloading function

• 상품 삭제, 등록, 조회 등 여러가지 요청을 request 메소드 하나를 오버로딩을 활용하여 작성하였습니다.

7. Test Doubles

• 테스트 작성을 위해 의존성 주입을 활용하여 Mock, Stub 객체를 만들어 활용하였습니다.
• URLProtocol을 상속받은 클래스를 만들고 재정의를 해주었습니다.
  • 이 방법은 URLSession의 dataTask를 직접 Stub으로 만드는 방법도 있었지만, URLSessionDataTask를 채택한 타입에 init()을 정의하니 deprecated 경고가 떠서 이를 해결하기 위해 삭제 후 URLProtocol을 활용하는 방법으로 로직을 변경하였습니다.

궁금했던 점 / 조언이 필요한 것

• 비동기 메서드를 사용하는 동기 메서드는 비동기 메서드 테스트로 진행해야할까요?
  • 저희가 긴가민가해서 NetworkManager를 테스트 할 때 비동기 테스트 시 필요한 메소드를 빼고 진행해보았는데, 실패를 해야하는데 성공을 하는 케이스가 나오더라구요. 그래서 일단 사용을 해주는게 맞는 것 같아서 그렇게 진행했는데, 이게 올바른건지 잘모르겠어요.
• URLProtocol을 활용하여 Test를 진행
  • WWDC2018 를 참고하여 URLProtocol을 상속받은 MockProtocol을 구현하여 test를 진행했는데요.
  • 야곰닷넷 Test에 오동나무가 알려주신 네트워크 Test 방식과 구글에 나온 다수의 방식은 StubURLSessionDataTask를 구현하는 방법이었습니다.
  • 하지만 문제가 발생하여 URLProtocol을 커스텀 하는 방식으로 바꾸었는데요. URLProtocol 개념이 낯설어서 저희가 제대로 구현을 한건지 또 현업에서 네트워킹 Test를 어떤 방식으로 구현하는지 궁금합니다!
    (문제에 대한 자세한 사항은 README에 Trouble Shooting부분에 1번문항을 참고 부탁드립니다!)
• Health Checker의 필요성을 모르겠습니다..
  • API 문서를 들여다보면 맨 위에 Health Checker가 있는데, 이걸 어디에 활용하면 좋을지 몰라서 따로 Request를 구현하진 않았는데요. 혹시... 보통 어디서 쓰이는지가 궁금해서 여쭤봅니다.
• 테스트 시 Request의 바디도 체크를 해야할까요?
  • 테스트할 때 Request의 바디가 잘 들어갔는지 체크를... 해야할까요? 하고싶었는데, 뭔가 테스트하기 애매한 것 같아서 과감히 제외하고 테스트를 해주긴 했습니다. 그렇지만 이게 맞는건지 잘 판단이 서질 않아서 그린의 조언이 필요합니다!

해결이 되지 않은 점

• 실제 API를 활용해서 서버에 요청을 보낼 때를 테스트해보진 못했습니다. 요청시 identifier가 필요한데요. 아직 배정받은 identifier가 없어서 임의로 하드코딩 상태로 남겨두었어요. 나중에 identifier를 배정받게 되면 수정할 예정인데, 이 부분은 파라미터로 전달받는게 좋을까요?

// DELET - 상품 삭제
func request(id: UInt, secret: String) -> URLRequest? {
    ...
		// 이부분이요!!!!!
    request.addValue("80c47530-58bb-11ec-bf7f-d188f1cd5f22", forHTTPHeaderField: "identifier")
    
    return request
}

[GREENOVER]

@leeari95 @llghdud921
안녕하세요 아리 제리!!
이번 프로젝트에서 처음 뵙네요😁
2주간 잘 부탁드립니다🙇🏻
처음에 러닝커브도 있는 쉽지 않은 프로젝트인데 너무 상세하게 아주 잘 구현해주셔서 놀랐어요🤭
본의 아니게 재밌게(?) 리뷰하다보니 코멘트들이 다소.. 아니 매우 많아졌어요ㅠㅠ...
저도 캠퍼해봤던 입장이라 리뷰 코멘트가 많으면 좋긴하지만 반면 짜증도 났기에 누구보다 그 입장을 잘 알기에..
우선 죄송합니다😭
다만 많은 코멘트에서 거의 대다수는 수정 요청이 아닌 정말 궁금해서 의견을 묻는 요지들이 많습니다.
(코드와 아주 별개의 것도 있구요..🥲)
그럼 리뷰를 너그럽게 봐주시면 감사하겠습니다🙌

---

칭찬 드리고 싶은 부분

• SwiftLint 룰을 아주 잘 도입해보셨네요! 아주 유용할거에요 💯
• 파일 그룹핑이 아주 세세하게 잘 되어 있어 구조 파악에 편했습니다 💯
• 각 역할들을 최대한 잘게 쪼개 가독성이 좋았어요 💯
• 테스트 코드를 기가 막히게 디테일하게 짜셨군요 💯
• 리드미 작성 정말 나중에 보답 받으실거에요 💯
• 쉽지 않으셨을텐데 대체적으로 통신과 모델링 관련해서 잘 구현해주셨네요! 💯

---

고민하셨던 부분에 대한 저의 의견

1. 단일 책임 원칙(Single responsibility principle)

• 이 부분이 아주 잘 되어 있으신거 보고 사실 좀 놀랐습니다🤭
  정말 객체지향을 잘 알고 설계하신것 같았어요. 이 부분은 고민하신만큼 성과가 나왔던 부분인것 같네요!

2. CodingKeys 활용

• 요 부분에 대해선 리뷰 코멘트로도 몇가지 남겨보았어요..!
  스네이크 케이스의 변환만 카멜 케이스로 가져가고 싶다면 코딩키 전략을 사용해도 됩니다ㅎㅎ
  또한 모델링에서 required 해야하는 부분인지도 남겨보았습니다!
  (대다수의 자세한 코멘트는 이곳보다 코드 라인바이라인으로 남겨봤어요ㅎㅎ
  혹시 이곳에 더 상세히 남기는게 더 편하시면 말씀해주세요!)

3. NetworkManager와 Network

• 궁금한 부분이 있습니다!
  왜 나눠주게 되었는지 기준이 궁금해요 사실🙋🏻
  테스트의 목적을 생각하신걸까요?
  아 저는 의견이 달라서 여쭤보는건 아닙니다!
  실제 통신은 결국 네트워크 매니저를 통해 이뤄지는데 Networkable 프로토콜과 그걸 따르는 Network 타입을 만들어 거쳐 사용함으로
  얻는 이점으로 어떤걸 생각하셨는지 궁금해요~!

4. Name Space

• 정말 아주 최고에요👍
  최대한 String Literal을 직접 코드에서 사용하는 지양하면 실수도 줄이고 개발자 에러를 줄일 수 있어서 저도 그렇게 사용합니다🙌

5. Request, Response

• 아주아주 잘 모델링되어 있는 부분 확인했습니다!
  다만 궁금한게 파일 그룹명이 모호한것 같아요.
  사실 그룹안에 들어있는 타입들이 상품을 위한것 같아서 이를 아우룰수 있는 폴더 네이밍은 어떨까 생각이 들었어요🤔
  Request / Response의 그룹 네이밍만 봐서는 하위 파일들을 보지 않고선 유추하기가 힘들것 같다고 느껴졌습니다.
  그 하위 파일들을 각 모델로 나누고 구현하신 부분은 좋은것 같습니다!

6. Overloading function

• 이 부분도 궁금한걸 리뷰 코멘트에 적긴했습니다🙋🏻
  요약하자면 "개발자가 네이밍이 같아 파라미터로 구분지어줘야하는 구현에서 실수할 여지가 있지 않을까?" 였습니다🧐

7. Test Doubles

• 테스트 저도 약한 부분인데 두분이 너무 잘 구현해주셨더라구요!
  Mock과 Stub의 사용도 적절해보이구요.
  아마 오동나무가 테스트 더블 관련해서 강의를 만들었던(?) 맞는지 모르겠지만 아니라면 준비하고 있을거라 생각되고..!
  그걸 보셨는가 싶지만 이와 별개로 저는 test double에 대해 개념을 이 레퍼런스로 잡았던 기억이 있어 공유드립니다🙋🏻
  https://blog.pragmatists.com/test-doubles-fakes-mocks-and-stubs-1a7491dfa3da?gi=e7d32c619169
  저는 현업에서 fake 구현을 가장 많이 사용해요.
  stub 테스트는 결국 원하는 도출값을 내놓고 시작하기에 실제 통신을 타는지 어떤지 확인할때 모호합니다.
  이에 fake 구현은 가장 실제 통신 및 로직을 다 처리해보지만 속은 fake인 그런 기능을 해주기에 어떻게 보면 가장 테스트 다운 테스트라고 볼 수 있어요.
  한번 참고해보시면 도움이 되실거에요..!🙋🏻

---

궁금 혹은 조언 요청에 대한 저의 의견

1. 비동기 메서드를 사용하는 동기 메서드는 비동기 메서드 테스트로 진행해야할까요?

• 정말 죄송한데 제가 이 질문을 정확하게 이해를 못했어요ㅠ..
  조금 더 구체적으로 예시 혹은 설명을 주시면 다시 답변을 드려보겠습니다!
  백프로 이해가 되지 않은 상태에서 유추해서 답변 드리면 더 혼란만 드릴것 같아서요🥲

2. URLProtocol을 활용하여 Test를 진행

• 지금 같이 구현하는 방법은 저도 사실 처음봤어요!
  왜냐면 저의 경우는 통신 관련해서도 대개 라이브러리를 쓰기에 URLSession 애플 라이브러리로 통신을 구현하는것은 캠프 이후로 많이 없습니다.
  (보통 Moya, Alamofire를 쓰기에)
  그렇기에 저의 현업에서도 두 라이브러리로는 테스트도 조금 더 수월하게 진행할 수 있습니다.
  다만 외부 라이브러리라도 내부 구현은 애플 라이브러리 기조를 벗어날 수 없기에 상동하다고 생각합니다.
  그래서 두분이 트러블슈팅에 잘 나타내주신것을 토대로 저도 학습을 해봤는데 지금 구현에 문제는 없다고 판단됩니다!
  다만 해당 프로토콜을 채택하면서 사용되지 않는 메서드들도 오버라이드 한 부분이 있다고 보여서 리뷰 남겨봤습니다..!

3. Health Checker의 필요성을 모르겠습니다.

• 사실 구현하기 나름이겠지만 현재 두분이 구현하신 코드에선 필요하지 않습니다.
  Health Checker는 보다 네트워크 상태 모니터링에 필요한 부분입니다.
  지속적으로 서버에 신호를 보내 응답이 오는지에 대해 체크하는 부분을 구현하고 이를 통해 잘못된 응답이 온다면
  잘못된 리퀘스트가 아니라면 사실 서버가 죽은거겠죠?
  그럴경우 바로 즉각적인 대응을 해줄 필요가 있어 그럴 경우 보통 사용되는걸로 알고 있습니다.
  다만 클라에서 저도 그렇고 이 헬스 체커를 사용해본 기억은 없네요!
  (물론 이를 가지고 필요하다면 구현을 해줄 순 있겠지만요..!)
  그렇기에 지금처럼 필요하신 부분만 API문서에서 가져가셔도 무방합니다🙌

4. 테스트 시 Request의 바디도 체크를 해야할까요?

• 음 제 의견은 굳이 안해도 될것 같습니다!
  테스트 코드는 정말 스타일의 차이이긴 하지만 굳이 저거까지 체크해야될까? 라는 두분과 동일한 생각이 있습니다.
  어차피 request body를 포함해 정상적으로 request 되었다면 그에 대한 응답을 확인하면 되는 부분일것 같아요.

---

해결되지 않은 부분에 대한 저의 의견

• 두가지 스탠스를 취할 수 있을것 같습니다🙋🏻
  저 Identifier가 변치 않는것이라면 전역적으로 상수처리할수 있을것 같고
  그게 아니라면 말씀하신것처럼 파라미터로 받을것 같습니다!
  그런데 제가 알기론 보통 고유값일테니 상수로 받는게 어떨까 합니다!

---

두분 너무 고생많으셨어요..!
다시 한번 리뷰 코멘트가 너무 심각하게 많은것 같아 죄송스럽네요..😅
의견이 다른 부분은 바로바로 말씀해주세요ㅎㅎ
아 두분의 구현에서 다음 스텝으로 넘어가는데 지장은 없어 보입니다.
그렇지만 코멘트가 있다보니 한번 확인해보시고 수정할 부분은 수정 후 리뷰 재요청을
그게 아니면 머지 요청을 해주세요~!🚀

[GREENOVER]

얘는 필요한 코드인가요?🤔

[llghdud921]

이 코드는 NetworkManager의 fetch()의 디코딩하는 부분을 테스트하기 위해 decodable은 타입이지만 올바르지 않은 가짜 객체를 넣어 실패 케이스을 의도적으로 구현하였습니다.

// test하기 위한 부분
let parsingResult = parser.decode(source: data, decodingType: decodingType)

// test code
func test_Fetch_Decode_failure() {
let response = HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil)
...
let decodingtype = StubProduct.self

[leeari95]

그린!! 와우... 이렇게 코멘트로 뚜둘겨 맞아본 것은 처음이라... 행복합니다. 😁 🍏
꼼꼼하게 봐주시고 피드백해주신 덕에 수정사항이 많네요!!!
정말 감사합니다! 수정사항 간단히 정리하자면 아래와 같습니다.
문제 없다면 머지 부탁드리겠습니다. 감사합니다!
새해복 많이 받으세요~ 🙇🏻‍♀️
(테스트의 타겟, 린트 룰 추가 등은 다음 스텝에서 개선해서 수정해보도록 하겠습니다. 감사합니다!!!)

수정사항

• 테스트 코드에 중복되는 부분을 개선
  • 빠진 주석 및 줄바꿈을 수정
• Image의 프로퍼티 네이밍을 명확하게 수정
• 하드코딩 되어있는 문자열을 따로 enum 타입으로 빼주어 개선
• 에러의 네이밍을 명확하게 개선
• Parser, Parsable의 네이밍을 JSON을 덧붙혀 명확하게 개선
• 접근제어가 붙어있지 않은 프로퍼티에 접근제어를 추가
• Address의 네이밍을 명확하게 개선 (APIAddress)

[GREENOVER]

변경 사항 확인했습니다!
한가지 부분에 코멘트를 추가해봤어요🥲
그 외 전부 이상없고 아주 잘 구현해주셨습니다!
스텝 1은 이정도면 충분할것 같아 어프로브 후 머지하겠습니다~
변경사항이 생기는 부분은 마이너하기에 스텝2를 하시면서 같이 보완해보면 될것 같습니다!
고생하셨습니다🙌

++ 아실 수 있지만 깃헙에서 리뷰 재요청을 할때 우측 상단에 Reviewer 탭에서 새로고침 버튼을 눌러주시면
저에게 깃헙으로 재요청 알림이 갑니다!
활용해보시면 편할거에요ㅎㅎ

p.s. 아 커밋 메시지 규칙에서 궁금한게 하나 있는데요..!
두분은 단순 네이밍만 변경되는 커밋에는 어떤 prefix를 달아주고 계신가요?
현재는 refactor를 달아주는것으로 보이는데 이유가 궁금합니다🙋🏻

[GREENOVER]

force Unwrapping 타입말고 조금 더 안전하게 사용해볼 수 있을까요?
테스트 코드여서 실제 앱 환경에서 문제는 없겠지만 forece Unwrapping은 항상 앱 크래쉬의 주요 원인이 됩니다🥲

[leeari95]

테스트 코드여서 상관없다고 생각해서 느낌표를 붙여주었는데, 그린 피드백대로 안전하게 사용해보도록 하겠습니다! 😊