feat: 네트뭐크 모듈 추가

[hryeong66]

What is this PR? 🔍

이슈

• [Feature] Network 모듈 개발 #4

설명

• 네트뭐크 모듈 추가했습니답
• 우선 example도 추가해두었습니다

Changes 📝

To Reviewers 🙏

EndPoint 관래서 고민이 되는 부분이 있는데여
Requestable에서 정의하는 프로퍼티들이 각 EndPoint에도 동일하게 들어갈거 같아서

public protocol Requestable {
  var url: String { get }
  var httpMethod: HTTPMethod { get }
  var headers: [String: String]? { get }
  var body: Data? { get }
  
  func buildURLRequest(with url: URL) -> URLRequest
}

1. EndPoint들이 Requestable을 채택하게 하는게 좋을까?

• NetworkRequest 객체를 따로 만들지 않고 EndPoint들이 Requestable을 바로 채택하는게 어떨까

2. Endpoint를 request 객체로 만드는게 좋을지
   → 이렇게 되면 https://github.com/mash-up-kr/MOIT-iOS/blob/develop/MOITNetwork/Interface/Endpoint.swift
   이런 방식으로 바꾸는게 좋지 않을까 합니당

의견 팍팍 얘기해주시면 감사드립니당🙇‍♀️

[chansooo]

너무 수고 많으셨습니다~

고민하시는 부분은 어떻게 구현하는지에 따라 둘 다 괜찮게 보이는데요,
각각의 프로토콜과 객체가 하는 역할이 무엇인지, 중복되는 역할은 없는지를 고민하시고, 원하시는 방식대로 하셔도 좋을 듯 합니다 ㅎㅎ

아래 링크는 네트워크 모듈 코드인데, 혹시나 참고가 될까 싶어서 남겨드립니다.

https://github.com/GongGanGam/GongGanGam-iOS/tree/develop/Modules/Network/Sources

[chansooo]

도메인 로직에서 사용되는 엔티티의 경우는 추후에는 VO를 떼도 무방할 것 같습니다 ㅎㅎ

[hryeong66]

요 도식으로 보면 제가 처음에 생각했던게 responseModel → DTO, entity → VO로 생각했었는데
DTO, VO로 표현하는게 더 헷갈리는 것 같아서 ExampleResponseModel, ExampleEnity로 네이밍 변경했습니다 a6a73b0

[jongnan]

이름을 풀어서 생각해보면 더 좋을꺼 같아요

DTO -> Data Transfer Object
VO -> Value Object

결국 둘 다 한 레이어에서 다른 레이어로 넘어갈 때 사용하는 것이고, 뮤터블하냐 이뮤터블하냐의 차이인거 같아요

entity가 중간에 변경이 될 수 있는 여지도 있다는 얘기를 하고 싶었어요!
그렇게 된다면, 엔티티 자체가 다른 레이어로 넘어갈텐데 DTO가 될 수도 있다는?

결국, 중요한건 Entity는 도메인 로직에서 사용하게 될 핵심 객체들인 것이고 postfix를 붙히냐 마냐도 상관없지만 네이밍이 길어질 수 있는 여지도 있기 때문에 제 생각에는 안붙혀도 될꺼 같습니다.

[chansooo]

현재 코드에서는 200번 이외에는 serverError로 내려오기 때문에 200, 300, 400, 500으로 나누거나, associated value로 status code를 전달해야 사용하는 부분에서 어떤 에러가 발생했는지 알 수 있을 것 같습니다. .!

[GREENOVER]

디코딩 에러에 대해서도 구체적으로 오류를 던지면 반환 타입도 옵셔널할 필요없기에 조금 더 명확하게 오류 처리도 네트워크단에서 감지할 수 있어 보입니다 😃

[GREENOVER]

• 실제 에러 타입만 반환하는것보다 어떤 에러였는지 디버깅을 명확히 하기 위해 status code뿐 아니라 reponse, data 등도 info로 담아 처리해주면 더 보완될것 같네유!

[hryeong66]

NetworkError를 세분화 하고 return 을 (data: T?, error: Error?)를 해서
호출하는 쪽에서도 error를 받아서 확인할 수 있도록 수정했습니다!
제안 감사합니다!! 🙇‍♀️ b1e342a

[GREENOVER]

오호 👍 그럼 Result 타입으로 반환하면 좀 더 편리하지 않을까요~?

[chansooo]

NetworkRequest를 Requestable로 추상화를 하면서 얻는 이점이 어떤 것이 있을까요??

[hryeong66]

NetworkRequest 구조체를 따로 가지고 있을 필요가 없는 것 같아서
NetworkRequest를 삭제하고 Endpoint가 Requestable 채택하도록 변경했습니다! b1e342a

[chansooo]

좋습니다~
다만 현재 상황에서 body가 아닌 query로 넘길 때를 대비해서 추가를 하는 것이 좋아보이네요

[hryeong66]

오메나.. 요부분은 그 전에 참고 코드로 알려주신 부분 참고해서 수정했습니다!
988a6ae

[chansooo]

여러개의 endpoint가 생길 때 중복으로 구현되는 부분이 어느 부분이고, network모듈로 넘겨줄 때 인터페이스로 분리가 되어 있을까요??
Endpoint를 추상화해서 Netowork 모듈에서 두고 Requestable, NetworkRequest와의 역할을 한번에 하면 어떨까요?

[hryeong66]

넵! NetworkRequest 구조체를 한번 더 만드는 과정이 불필요한 것 같아서
NetworkRequest를 삭제하고 우선은 Endpoint가 Requestable을 채택하도록 수정했습니답!
음....제안주신 것 생각해보니 아예 Requestable → Endpoint로 네이밍을 변경하는 것도 괜찮을 것 같은데 어떤가욤??

[chansooo]

이름 자체는 requestable과 endpoint 둘 다 괜찮아 보입니다.
구현해주신대로 requestable을 protocol로 두고, 해당 프로토콜을 채택하는 객체들을 endpoiint로 정의해도 괜찮구요.
편하신대로 해주시면 될 것 같습니다.

[chansooo]

repository과 apiclient의 각각의 역할이 무엇일까요?? 왜 나눠져야 할까요???

[hryeong66]

요거는.. 제가 잘못짠거 같아여🥲 repository과 apiclient가 같은 역할을 하고 있어서 apiclient을 삭제하고 repository에서 apiService request를 호출하도록 수정했습니다! f340e5a

[chansooo]

repository 에서 해당 apiclient를 composition하고 사용하는 이유가 있을까요??? 두 객체의 역할의 차이가 있을까요???

[hryeong66]

위 코멘트처럼 두 객체의 역할 차이가 없어서... apiclient을 삭제했습니다!! f340e5a

[chansooo]

NetworkServiceable과 ExampleServiceable은 각각의 역할에 차이가 있어 보이는데, 명확하게 구별할 수 있는 방법이 있을까요???

[jongnan]

단순 궁금증인데 프로토콜의 postfix로 able을 사용하는건가요?

[hryeong66]

얘기해주신 것 처럼 역할은 다른데 네이밍이 비슷해서
ExampleServiceable → ExampleRepository로 네이밍 변경했습니다!

@jongnan 넴~! 스위프트 가이드에 따르면, ~할 수 있는 protocol을 정의하는 경우에
swift에서 Decoable, Equatable 처럼 able을 많이 붙이는 것 같아요!
https://www.swift.org/documentation/api-design-guidelines/
Protocols that describe a capability should be named using the suffixes able, ible, or ing (e.g. Equatable, ProgressReporting).

그래서 실제 api Request를 구현할 쪽을 NetworkServiceable
request 객체를 생성하는 쪽을 Requestable로 지었습니다!
이렇게 보니까 ExampleServiceable 자체 네이밍이 넘 구렸네여.. 🥲

[jongnan]

이 반대도 있으면 좋지 않을까요?

String.toObject? String.toDTO?

[chansooo]

@jongnan
String 자체를 DTO로 만드는 경우는 비즈니스 로직에서 어떤 부분에서 필요할까요???

[jongnan]

아 body 파싱할 때 사용하면 되겠다고 생각했었는데요!

생각해보니까 Decodable로 정의하고 파서가 따로 있었네요!

[jongnan]

해당 함수는 기본형에 해당하는건가요?
프로토콜을 여기서 구현하는 이유는 무엇인가요??

[hryeong66]

해당 프로토콜을 채택하는 곳에서 공통으로 사용할 수 있게 extension에서 구현했습니다!
swift에서는 프로토콜이나 상위 타입에 사용하고 싶은 공통 함수가 필요하다면 extension에서 주로 구현하는 것 같아요ㅎㅎ

public extension Double {
    var toString: String {
        return String(self)
    }
// UILabel 타입과 이를 상속받는 곳에서도 모두 사용가능, 해당 함수를 재정의하싶다면
// override  func strikeThrough() {} 구현 
public extension UILabel {
 func strikeThrough() {
        let attrString: NSAttributedString?
        if let attributedText {
            attrString = attributedText.strikeThrough(self.textColor)
        } else {
            attrString = self.text?.strikeThrough(self.textColor)
        }
        self.attributedText = attrString
    }
}

만약 채택하는 쪽에서 해당 함수를 오버라이딩처럼 쓰고 싶다면
override 키워드 없이 public func buildURLRequest(with url: URL) -> URLRequest 이 함수를 구현해서 쓰면 됩니당

[chansooo]

@jongnan
추상 클래스라 생각하심 될 듯 합니다 ㅎㅎ

[jongnan]

아하 답변 감사해요~

[hryeong66]

선생님덜 리뷰 감사합니다ㅏ🙇‍♀️

전체적으로 각 파일의 필요성과 네이밍을 다시 수정해서 정리했습니다!

• 불필요하게 apiClient가 중간에 들어가 있어서 해당 클래스는 삭제하고 Respository에서 api request를 하도록 수정했습니답!
• 데이터 모델 네이밍은 responseModel → DTO, entity → VO로 생각했었는데
  DTO, VO로 표현하는게 더 헷갈리는 것 같아서 ExampleResponseModel, ExampleEnity로 네이밍 변경했습니다~!

[chansooo]

혹시 NetworkServicable로 추상화한 프로토콜을 두신 이유가 있을까요???

[hryeong66]

RepositoryImpl가 networkService를 가지고 있어야 하는데
그때 타입을 프로토콜로 해서 주입할 수 있게 하는게 좋지 않을까 했습니다!

[chansooo]

반환 타입이 tuple로 되어 있을 경우 (data, error)의 경우의 수가

1. nil, not nil
2. nil nil
3. not nil, not nil
4. not nil, nil
   총 4가지가 되게 됩니다.
   이 중에서 2와 3은 일어날 수 없는 경우의 수이구요.
   따라서 해당 부분을 Result 타입으로 변경해서 경우의 수를 2개로 줄이면 해당 객체를 사용하는 부분에서 핸들링을 하기 더 쉬워질 것 같습니다~

[hryeong66]

넵 해당 부분 Result 타입으로 변경했습니다 4b3c5cf

[chansooo]

네이밍은 마음대로 해도 되지만 domain layer에서 사용되는 모델들의 경우는 entity를 떼도 좋을 듯 싶습니다.
이건 추후에 같이 컨벤션 정하면서 이야기해보시죠 ㅎㅎ
e.g. PhotoEntity -> Photo

[jongnan]

#6 (comment)

위에서도 적었두었는데, 저도 찬수님의 의견의 동의합니다.

[hryeong66]

넵 좋습니다! 예시지만 entity 네이밍 제거했습니다 f1236a8

[chansooo]

예시이긴 하지만, network 모듈 내부에서 에러를 분기처리해서 던졌는데, data layer에서 nil로 반환을 해버리면 의미가 퇴색되는 것 같습니다!

[jongnan]

간단하게 서칭을 해봤는데요!

https://velog.io/@segassdc1/Logging%EC%97%90-%EB%8C%80%ED%95%9C-%EA%B3%A0%EC%B0%B0

이렇게 로거를 이용해서 에러를 찍는게 좀 더 디버깅할 때나 가시성에도 좋지 않을까요?

[hryeong66]

제안 감사합니다!! 요거 활용해서 수정해볼게요!

[jongnan]

여기에는 able을 안쓴 이유가 있나요?
Repositable?

[chansooo]

able을 붙이는 이유가 하나의 동작에 대해서 protocol을 정의하고, 해당 동작을 수행하는 객체에 채택하는 용도인데요,
e.g. drawable이라는 protocol은 shape라는 필드와 draw()라는 함수를 가지도록.
즉, 동사의 뒤에 접미어가 붙는 것이 자연스럽습니다.
repository처럼 명사형인 경우에는 의미가 더 어색해질 수 있기 때문에 지금처럼 두고, 구현부를 ExampleRepositoryImpl 등으로 네이밍해도 괜찮을 것 같습니다.

[github-actions]

여기 코드 패치에 대해 간단히 코드 리뷰를 작성하겠습니다.

버그 위험

1. 중복된 모델 필드:

   • ExampleResponseModel와 ExampleEntity에 동일한 exampleString 변수가 있습니다. 두 모델이 꼭 필요한 경우 중복을 피해야 합니다.

2. 불필요한 커스텀 Decoding:

   • ExampleEntity에서 커스텀 디코딩 초기자가 사용되었지만, 기본 구조체 자동 초기화가 같은 역할을 수행할 수 있습니다.

개선 사항

1. 중복 제거:

   public struct ExampleModel: Decodable {
       public var exampleString: String?
   }

2. 심플한 초기자:

   • Swift의 구조체는 자동으로 멤버 초기자를 제공한다는 점에서 커스텀 초기자가 필요 없어 보입니다.

3. 코멘트 정리:

   • 위의 주석들이 불필요하게 많이 포함되어 있어 보기 어렵습니다. 중요 사항만 남기고 정리하면 가독성이 높아집니다.

4. 불필요한 Foundation Import 확인:

   • Foundation 모듈이 꼭 필요한지 다시 한 번 확인해보세요. 만약 필요 없다면 삭제할 수 있습니다.

수정된 코드 예시:

//
//  ExampleModel.swift
//  PPACModels
//
//  Created by 장혜령 on 6/9/24.
//

public struct ExampleResponseModel: Decodable {
    public var exampleString: String?
    public var exampleString2: String?
}

public struct ExampleEntity: Decodable {
    public var exampleString: String?
}

이와 같이 중복과 불필요한 코드를 줄이면 코드의 유지보수성과 가독성이 높아질 것입니다.

[hryeong66]

요건 예시에서 Example(exampleString: String) init 호출하려구 추가했습니답

[github-actions]

코드 리뷰:

긍정적인 점:

1. async/await를 사용하여 비동기 프로그래밍을 간결하게 처리.
2. 올바른 에러 핸들링(에러 로그 포함).

개선 사항 및 버그 위험:

1. Http 응답코드 검사:

   case 200..<300:
   if let decodedData = try? decoder.decode(T.self, from: data) {
       return .success(decodedData)
   } else {
       error = .dataDecodingError
   }

   여기서 try? 대신 do-catch 블록을 사용하는 것이 안전합니다. 그렇지 않으면 디버깅 시 문제가 발생할 수 있습니다.

   do {
       let decodedData = try decoder.decode(T.self, from: data)
       return .success(decodedData)
   } catch {
       error = .dataDecodingError
   }

2. 에러 메시지 처리:
   HTTP 오류에서 에러 메시지를 만들 때, String(data: data, encoding: .utf8)가 optional이라 nil일 가능성이 있습니다. 기본 값을 설정해 주는 것이 좋습니다.

   let message = String(data: data, encoding: .utf8) ?? "Unknown error"
   error = .clientError(statusCode: httpResponse.statusCode, message: message)

3. URLSession의 기본 설정:
   URLSession.shared를 사용할 때, 필요하면 사용자 정의 세션 구성을 활용하여 타임아웃이나 캐시 정책 등을 설정할 수 있도록 합니다.

4. 테스트 코드 추가 요청:
   기능이 제대로 동작하는지 확인할 수 있는 테스트 코드를 추가하면 좋습니다.

5. 타입 안정성 향상:
   함수 파라미터로 받는 Requestable과 NetworkError 타입 정의를 명확히 하는 것이 좋습니다. 이는 전체 코드의 가독성과 유지보수성을 높입니다.

---

위의 개선 사항들을 반영하여 더 견고하고 유지보수하기 좋은 코드를 작성할 수 있을 것입니다.

[github-actions]

여기 코드 패치에 대한 간단한 코드 리뷰입니다:

개선사항 및 잠재적 버그

1. makeURL 함수에서 return URL(string: url)?.append(queries: parameter) 부분:

   • append(queries:)가 옵셔널 URL을 반환하기 때문에 체이닝 결과가 옵셔널 옵셔널(URL??)이 될 가능성이 존재합니다. flatMap 혹은 명시적인 옵셔널 해제를 통해 이를 처리하는 것이 좋습니다.

2. buildURLRequest(with:) 함수의 urlRequest.httpMethod = httpMethod.rawValue.uppercased() 부분:

   • HTTP 메서드의 경우 대문자로 사용하는 것이 일반적이나, uppercased()로 변환하는 대신 HTTPMethod 열거형 정의 시 이미 대문자로 작성해 두는 것이 안전합니다.

3. 에러 처리 부족:

   • append(body parameter:)에서는 JSON 인코딩 실패 시 예외를 던지지 않고 무시합니다. 이로 인해 디버깅이 어려울 수 있습니다. 이를 적절히 처리하거나 로그를 남기는 것이 좋습니다.

4. URL 컴포넌트 생성 반복:

   • URLComponents(string: self.absoluteString)와 같은 컴포넌트 생성을 반복적으로 하지 않도록 할 수 있습니다.

5. 확장 메서드 네이밍:

   • append(queries:)와 append(body:)는 이름이 명확하지만, append라는 단어는 기본 동작과 밀접하게 연관되어 있을 수 있어 혼동을 일으킬 수 있습니다. 더 명확한 이름(예: appendingQueries 또는 addingQueryItems)을 사용하는 것을 권장합니다.

코드 수정 제안

extension Requestable {

  public func makeURL() -> URL? {
    guard let baseURL = URL(string: url) else { return nil }
    return baseURL.append(queries: parameter)
  }
  
  public func buildURLRequest(with url: URL) -> URLRequest {
    var urlRequest = URLRequest(url: url).appendingBody(parameter: parameter)
    urlRequest.httpMethod = httpMethod.rawValue.uppercased()
    urlRequest.allHTTPHeaderFields = headers ?? [:]
    return urlRequest
  }
}

extension URLRequest {
  
  // rename this function to be more descriptive
  func appendingBody(parameter: HTTPRequestParameter?) -> URLRequest {
    var request = self
    
    if case .body(let bodyParameters) = parameter {
      do {
        let encodedParameters = try JSONEncoder().encode(bodyParameters)
        request.httpBody = encodedParameters
      } catch {
        print("Error encoding parameters: \(error)")
      }
    }
    
    return request
  }
  
}

extension URL {
  // rename this function to be more descriptive
  func append(queries parameter: HTTPRequestParameter?) -> URL? {
    var components = URLComponents(string: self.absoluteString)
    
    if case .query(let queries) = parameter {
      let queryItems = queries.map { URLQueryItem(name: $0.0, value: $0.1) }
      components?.queryItems = queryItems
    }
    
    return components?.url
  }
}

이 코드 리뷰를 통해 향후 유지보수성과 에러 처리를 강화할 수 있습니다. 추가적인 네이밍 규칙 수정도 고려해보세요.

[github-actions]

다음은 제안된 코드 패치에 대한 간략한 코드 리뷰입니다:

1. 타이포 수정:

   • ExampleServiceEndpoints.postExmple에서 postExmple을 postExample로 수정해야 합니다.

2. 중복 코드 제거:

   • fetchExample 및 postExample 메서드의 공통된 부분을 함수로 분리하면 코드 중복을 줄일 수 있습니다.

   private func handleRequest<T: Decodable>(_ request: URLRequest, responseType: T.Type) async -> ExampleEntity? {
       let result = await self.apiService.request(request, dataType: responseType)
       switch result {
       case .success(let data):
           return ExampleEntity(exampleString: data.exampleString)
       case .failure(let error):
           return nil
       }
   }
   
   func fetchExample() async -> ExampleEntity? {
       let request = ExampleServiceEndpoints.fetchExample
       return await handleRequest(request, responseType: ExampleResponseModel.self)
   }
   
   func postExample(id: Int) async -> ExampleEntity? {
       let request = ExampleServiceEndpoints.postExample(id: id)
       return await handleRequest(request, responseType: ExampleResponseModel.self)
   }

3. 에러 처리:

   • 현재 에러를 무시하고 있으며, nil을 반환합니다. 나중에 디버깅 할때 어려울 수 있으므로 로그나 오류 전파 등의 방식으로 개선 필요합니다.

4. 불필요한 리턴문 제거:

   • postExample 메서드의 마지막 return 문은 불필요합니다. 이미 switch문 내에서 적절한 값을 반환하고 있습니다.

   func postExample(id: Int) async -> ExampleEntity? {
       let request = ExampleServiceEndpoints.postExample(id: id)
       return await handleRequest(request, responseType: ExampleResponseModel.self)
   }

5. 네이밍 컨벤션:

   • apiService 보다는 networkService와 같은 좀더 명확한 이름을 사용할 수 있습니다.

코드를 지속적으로 개선 및 최적화하는 것은 매우 중요합니다. 이러한 조언들이 도움이 되기를 바랍니다.

[github-actions]

코드 패치에 대해 리뷰를 간단히 하겠습니다:

버그 리스크:

1. 오타:

   • case postExmple(id: Int)에서 "postExmple"의 오타가 있습니다. "postExample"로 수정하는 것이 좋습니다.
   • httpMethod와 parameter에서도 동일하게 오타가 있어 수정이 필요합니다.

2. HTTP 메서드 오류:

   • postExmple(또는 postExample)의 HTTP 메서드가 .get으로 설정되어 있는데, 이는 POST 요청이어야 합니다. 이를 .post로 수정해야 합니다.

3. 기본 URL:

   • let baseUrl = "" 기본 URL이 빈 문자열로 설정되어 있습니다. 적절한 기본 URL을 정의해야 합니다.

개선 제안:

1. 리팩터링:

   • url에서 baseUrl 부분을 중복해서 사용하고 있으므로, baseUrl을 상수로 빼내어 관리하면 더 깔끔할 것입니다.

2. 타임아웃 설정:

   • requestTimeOut은 정수형으로 되어 있지만, enum이나 상수를 활용하여 더 명확하게 표현할 수 있습니다.

코드 수정 예시:

enum ExampleServiceEndpoints: Requestable {
  case fetchExample
  case postExample(id: Int)
  
  var requestTimeOut: Int {
    return 20
  }
  
  var httpMethod: HTTPMethod {
    switch self {
    case .fetchExample:
      return .get
    case .postExample:
      return .post // 수정됨
    }
  }
  
  var parameter: HTTPRequestParameter? {
    switch self {
    case .postExample(let id):
      return .query(["id": "\(id)"]) // 변수명 수정됨
    default:
      return nil
    }
  }
  
  var url: String {
    let baseUrl = "https://api.example.com" // 실제 baseUrl로 수정
    switch self {
    case .fetchExample, .postExample: // 두 케이스 통합
      return "\(baseUrl)/example"
    }
  }
  
  var headers: [String: String]? {
    var headers: [String: String] = [:]
    headers["Authorization"] = "token"
    headers["Content-Type"] = "application/json"
    return headers
  }
}

해당 사항들을 수정하면 코드가 더 효율적이고 명확해질 것입니다.

[github-actions]

확인하신 코드 패치에 대한 간단한 코드 리뷰와 개선 사항은 다음과 같습니다:

리뷰

1. 구조적 통일성 유지:

   • ExampleResponseModel과 Example 두 구조체가 있는데, 각각 Decodable을 채택하고 있습니다. 그러나 CodingKeys를 사용하는 것은 오직 Example 뿐입니다. 일관성을 위해 두 구조체 모두 동일한 패턴을 따르는 것이 좋습니다.

2. 불필요한 초기화 코드:

   • Example 구조체에 public init(exampleString: String? = nil) 이 초기화 메서드는 암묵적인 초기화를 제공하는 Swift의 특성상 불필요할 수 있습니다.

개선 제안

1. 일관성 유지:

   public struct ExampleResponseModel: Decodable {
     public var exampleString: String?
     public var exampleString2: String?
     
     enum CodingKeys: String, CodingKey {
       case exampleString
       case exampleString2
     }
   
     public init(from decoder: Decoder) throws {
       let container = try decoder.container(keyedBy: CodingKeys.self)
       exampleString = try container.decodeIfPresent(String.self, forKey: .exampleString)
       exampleString2 = try container.decodeIfPresent(String.self, forKey: .exampleString2)
     }
   }

2. 불필요한 초기화 제거:

   public struct Example: Decodable {
     public var exampleString: String?
     
     enum CodingKeys: String, CodingKey {
       case exampleString
     }
   
     public init(from decoder: Decoder) throws {
       let container = try decoder.container(keyedBy: CodingKeys.self)
       exampleString = try container.decodeIfPresent(String.self, forKey: .exampleString)
     }
   }

이러한 개선 방안을 통해 코드의 일관성과 가독성을 높일 수 있을 것입니다.

[github-actions]

아래는 코드 패치에 대한 간단한 코드 리뷰입니다:

1. 버그 위험:

   • makeURL() 메서드에서 url.append(queries: parameter)를 호출하기 전에 guard let url = URL(string: self.url)로 생성된 URL 객체를 사용하고 있어, 해당 URL이 인스턴스 변수 self.url과 혼동을 일으킬 수 있습니다.
   • append(body:) 메서드에서 JSON encoding이 실패할 경우 에러 처리가 제대로 이루어지지 않습니다. 이를 try-catch 구문으로 감싸는 것이 좋습니다.

2. 개선 제안:

   • makeURL()의 guard let url = URL(string: url) 부분에서 URL 변수를 사용할 때 이름 충돌을 피할 수 있도록 다른 이름을 사용하는 것이 좋습니다.

     guard let constructedURL = URL(string: self.url) else { return nil }

   • buildURLRequest(with url:) 메서드 내에서 headers ?? [:] 구문을 보다 명확하게 하기 위해 기본 값을 설정하는 것이 좋습니다.

     urlRequest.allHTTPHeaderFields = headers?.isEmpty == false ? headers : ["Content-Type": "application/json"]

   • append(body:) 메서드에서 JSON encoding 실패 시 로그나 Error throw 구문 추가.

     if case .body(let bodyParameters) = parameter {
         do {
             let encodedParameters = try JSONEncoder().encode(bodyParameters)
             request.httpBody = encodedParameters
         } catch {
             // Error handling (e.g., print or throw)
             print("Failed to encode parameters: \(error.localizedDescription)")
         }
     }

   • append(queries:) 메서드에 query 파라미터가 없을 경우도 고려하여 기본 값을 할당할 수 있습니다.

     if case .query(let queries) = parameter, !queries.isEmpty {
         let queryItems = queries.map { URLQueryItem(name: $0.key, value: $0.value) }
         components?.queryItems = queryItems
     }

3. 형식 개선:

   • 코드 주석이나 문서를 추가해서 각 메서드나 클래스를 더 잘 설명하면 이해하기 쉬워집니다.
   • 각 파일 및 코드 블록의 끝에 공백 라인을 추가하면 파일 끝부분이 깔끔해집니다.

위의 제안을 바탕으로 코드를 수정하면 안정성과 가독성이 크게 향상될 것입니다. 좋은 코드 작성하시기 바랍니다!

[github-actions]

처음 코드를 리뷰한 결과, 몇 가지 개선 사항과 잠재적인 버그 리스크를 발견했습니다.

개선 사항

1. 중복된 코드 제거: fetchExample과 postExample 메서드 내의 성공 케이스가 동일합니다. 이 부분을 별도의 함수로 추출하는 것이 좋습니다.
2. 오타 수정: ExampleServiceEndpoints.postExmple(id: id)에서 "postExmple"을 "postExample"로 수정해야 합니다.
3. 불필요한 반환문 삭제: postExample 끝에 위치한 return Example(exampleString: "")는 불필요합니다. 스위치 문 외부에서는 이미 모든 경로에서 값을 반환하므로 제거할 수 있습니다.

잠재적인 버그 리스크

• 실패 시 예외 처리 부족: 실패 케이스에서 에러 로그 혹은 추가적인 오류 처리가 필요해 보입니다. 현재는 단순히 nil을 반환하고 있으며 사용자는 원인을 파악하기 힘들 수 있습니다.
• 데이터 타입 미스매치: 네트워크 요청 시 dataType: ExampleResponseModel.self 타입이 지정되어 있는데, 이 타입이 실제 Example과 어떻게 연결되는지 명확하지 않아 보입니다. 문제가 발생할 소지가 있습니다.

수정된 코드 샘플

//
 //  ExampleRepository.swift
 //  Home
 //
 //  Created by 장혜령 on 6/9/24.
 //

import Foundation
import PPACModels
import PPACNetwork

protocol ExampleRepository {
    func fetchExample() async -> Example?
    func postExample(id: Int) async -> Example?
}

class ExampleRepositoryImpl: ExampleRepository {

    let networkService: NetworkServiceable
    init(networkService: NetworkServiceable) {
        self.networkService = networkService
    }
    
    private func handleResult(_ result: Result<ExampleResponseModel, NetworkError>) -> Example? {
        switch result {
        case .success(let data):
            return Example(exampleString: data.exampleString)
        case .failure(let error):
            // TODO: 에러 로깅 및 추가 처리
            print("Error: \(error)")
            return nil
        }
    }

    func fetchExample() async -> Example? {
        let request = ExampleServiceEndpoints.fetchExample
        let result = await self.networkService.request(request, dataType: ExampleResponseModel.self)
        return handleResult(result)
    }

    func postExample(id: Int) async -> Example? {
        let request = ExampleServiceEndpoints.postExample(id: id) 
        let result = await self.networkService.request(request, dataType: ExampleResponseModel.self)
        return handleResult(result)
    }
}

이와 같이 변경하면 코드의 중복성을 줄이고, 가독성을 높일 수 있을 것입니다.

[github-actions]

여기 코드에 대한 리뷰를 드립니다:

버그 위험성:

1. 중복된 코드: ExampleResponseModel과 Example 두 모델의 구조가 매우 유사합니다. 이는 유지보수 시 혼동을 초래할 수 있습니다.
2. 옵셔널 사용 주의: 모든 프로퍼티가 옵셔널로 되어 있는데, 이로 인해 예기치 않은 nil 값이 발생할 수 있습니다. 가능한 경우 기본 값을 할당하거나 옵셔널 사용을 최소화 하는 것이 좋습니다.

개선 제안:

1. 모델 통합 또는 상속: 만약 ExampleResponseModel과 Example 간의 중복을 줄여야 한다면, 동일한 모델을 사용하거나 공통 속성을 갖는 슈퍼클래스를 만들 수 있습니다.

   public struct ExampleBase: Decodable {
     public var exampleString: String?
   }
   
   public struct ExampleResponseModel: Decodable {
     public var exampleString: String?
     public var exampleString2: String?
   }
   
   public struct Example: ExampleBase {
     // Example-specific properties
   }

2. Decodable 초기화 간소화: Decodable 프로토콜을 준수하기 위해 이미 자동으로 생성되는 초기화 코드를 추가로 작성할 필요는 없습니다. 아래와 같이 간단히 쓸 수 있습니다.

   public struct Example: Decodable {
     public var exampleString: String?
     
     private enum CodingKeys: String, CodingKey {
       case exampleString
     }
   }

3. Foundation import 위치 조정: 일반적으로 Foundation은 가장 최상단에 두는 것이 관례입니다.

수정된 코드 예시:

import Foundation

public struct ExampleResponseModel: Decodable {
  public var exampleString: String?
  public var exampleString2: String?
}

public struct Example: Decodable {
  public var exampleString: String?

  enum CodingKeys: String, CodingKey {
    case exampleString
  }
}

더 필요한 정보나 질문사항이 있다면 알려주세요!

[github-actions]

코드 리뷰 요약 (한국어):

1. URLSession Handling:

   (data, response) = try? await URLSession.shared.data(for: urlRequest)

   try? 대신 try를 사용하고 catch 블록에서 에러를 처리하는 것이 더 명확합니다. 현재 코드에서는 네트워크 오류가 발생해도 캐치되지 않고 이후 코드로 넘어갑니다.

2. Error Logging:
   모든 오류 케이스에 대해 NetworkLogger.logError를 호출하고 있습니다. 이런 점은 좋지만, 상세하게 로그 메시지를 남기면 디버깅이 더 용이할 수 있습니다.

3. Data Decoding:
   데이터 디코딩을 실패한 경우에 대한 에러 처리가 필요합니다.

   if let decodedData = try? decoder.decode(T.self, from: data) {
       return .success(decodedData)
   } else {
       error = .dataDecodingError
   }

   여기서 try?보다는 do-catch 형태로 디코딩 에러를 명확히 캐치하는 것이 좋습니다.

4. 함수 이름: 함수명이 request인데 이는 너무 일반적입니다. REST API 요청 관련 클래스라면 조금 더 구체적인 이름으로 변경하면 가독성이 향상될 수 있습니다.

개선된 예:

public func sendRequest<T: Decodable>(_ request: Requestable, responseType: T.Type) async -> Result<T, NetworkError> {

종합적으로 코드 구조는 일관성 있고 잘 작성되었습니다. 그러나 비동기 요청과 오류 처리 부분에서 조금 더 명확한 에러 핸들링이 필요합니다.

[github-actions]

코드 리뷰에 대한 몇 가지 제안과 잠재적 버그 위험은 다음과 같습니다:

1. 안전한 옵셔널 처리:

   • makeURL() 메소드와 append(queries:) 확장에서 URL을 생성하는 방식이 안전하지 않을 수 있습니다. URLComponents 초기화가 실패할 경우를 대비해 디폴트 URL을 반환하거나, 오류를 적절히 처리하는 것이 좋습니다.

2. 메소드 명명:

   • append(queries:)와 append(body:)는 실제로 새로운 값을 반환하지 않고 수정된 객체를 반환하므로, 이 메소드들이 사이드 이펙트 없이 새로운 객체를 반환하도록 하는 것이 좋습니다. 이렇게 하면 코드를 읽기 쉽게 할 수 있습니다.

3. URLRequest 바디 인코딩:

   • append(body:) 메소드에서 JSONEncoder().encode(bodyParamters) 부분은 try? 구문으로 인해 실패 시 조용히 nil을 반환합니다. 에러를 철저히 로깅하고 대응하는 것이 좋습니다.

4. 주석:

   • 메소드와 프로토콜 정의에 대한 설명 주석을 추가하여 코드의 의도를 더 명확히 하면 좋습니다.

5. HTTPMethod에서 PATCH 지원 고려:

   • 일반적으로 많이 사용되는 PATCH 메소드도 지원하도록 HTTPMethod enum에 추가하는 것이 좋습니다.

6. 타입 선언 가시성:

   • url, httpMethod, headers, parameter 등 Requestable 프로토콜 프로퍼티의 접근 제한자가 명시되지 않았습니다. 인터페이스 정의에 따라 public 또는 다른 접근 제한자를 명시하는 것이 좋습니다.

수정된 예제 코드:

import Foundation

public enum HTTPMethod: String {
  case get
  case post
  case put
  case delete
  case patch
}

public enum HTTPRequestParameter {
  case query([String: String])
  case body(Encodable)
}

public protocol Requestable {
  var url: String { get }
  var httpMethod: HTTPMethod { get }
  var headers: [String: String]? { get }
  var parameter: HTTPRequestParameter? { get }

  func makeURL() -> URL?
  func buildURLRequest(with url: URL) -> URLRequest
}

extension Requestable {
  public func makeURL() -> URL? {
    guard let url = URL(string: url) else { return nil }
    return url.appending(queries: parameter)
  }

  public func buildURLRequest(with url: URL) -> URLRequest {
    var urlRequest = URLRequest(url: url)
      .appending(body: parameter)
    urlRequest.httpMethod = httpMethod.rawValue.uppercased()
    urlRequest.allHTTPHeaderFields = headers ?? [:]
    return urlRequest
  }
}

extension URLRequest {
  func appending(body parameter: HTTPRequestParameter?) -> URLRequest {
    var request = self

    if case .body(let bodyParameters) = parameter {
      do {
        let encodedParameters = try JSONEncoder().encode(bodyParameters)
        request.httpBody = encodedParameters
      } catch {
        print("Failed to encode body parameters: \(error)")
      }
    }

    return request
  }
}

extension URL {
  func appending(queries parameter: HTTPRequestParameter?) -> URL? {
    var components = URLComponents(string: self.absoluteString)

    if case .query(let queries) = parameter {
      let queryItems = queries.map { URLQueryItem(name: $0, value: $1) }
      components?.queryItems = queryItems
    }

    return components?.url
  }
}

이 코드는 각 메소드가 더욱 명확하고 안전하게 동작하도록 합니다.

[github-actions]

• postExample 함수에서 반환하는 값이 두 번 정의되어 있습니다. case .failure(let error) 이후, return Example(exampleString: "")는 불필요합니다.
• fetchExample와 postExample 모두 실패 시 오류 처리를 단순히 nil을 반환하는 것으로 하고 있는데, 좀 더 명시적인 오류 처리가 필요할 수 있습니다.
• postExmple 오타가 있습니다. postExample로 수정해야 합니다.

func postExample(id: Int) async -> Example? {
  let request = ExampleServiceEndpoints.postExample(id: id)
  let result = await self.networkService.request(request, dataType: ExampleResponseModel.self)
  switch result {
  case .success(let data):
    return Example(exampleString: data.exampleString)
  case .failure(let error):
    print("Error: \(error)")  // 혹은 보다 구체적인 오류 처리
    return nil
  }
}

개선 사항:

1. 오류 보고를 위한 로깅 추가
2. 요청 실패 시 좀 더 의미 있는 상태 반환

이러한 수정으로 코드가 가독성이 높아지고 유지보수에 용이해집니다.