Conversation
opficdev
commented
Feb 28, 2026
opficdev
commented
- closed 데이터 <-> 인프라 간 DTO 및 Mapper를 확인한다 #148
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 데이터와 인프라 계층 간의 DTO 및 매퍼 구조를 재정비하여 아키텍처의 명확성을 높이고 Firebase 종속성을 줄이는 데 중점을 둡니다. DTO 파일들을 Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
전반적으로 훌륭한 리팩토링입니다. 데이터와 인프라 계층을 분리하여 Firebase 종속성을 제거하고, DTO와 Mapper를 Data 계층으로 이동시킨 점이 인상적입니다. 이는 클린 아키텍처 원칙에 부합하며 코드의 테스트 용이성과 유지보수성을 크게 향상시킬 것입니다. Infra 계층의 서비스(TodoService, PushNotificationService 등)가 Firebase 데이터 모델을 애플리케이션 DTO로 변환하는 책임을 갖도록 수정한 것은 매우 올바른 방향입니다. 몇 가지 추가적인 개선 사항을 제안드립니다.
| struct PushNotificationResponse: Decodable { | ||
| @DocumentID var id: String? | ||
| struct PushNotificationResponse { | ||
| let id: String? |
There was a problem hiding this comment.
PushNotificationService의 makeResponse 함수를 보면 snapshot.documentID를 사용하여 id를 초기화하고 있습니다. documentID는 nil이 될 수 없으므로, id 프로퍼티를 String? 대신 String으로 선언하여 타입 안정성을 높이는 것이 좋습니다.
| let id: String? | |
| let id: String |
DevLog/Data/DTO/TodoDTO.swift
Outdated
| } | ||
|
|
||
| struct TodoResponse { | ||
| let id: String? |
There was a problem hiding this comment.
TodoService의 makeResponse 함수를 보면 snapshot.documentID를 사용하여 id를 초기화하고 있습니다. documentID는 nil이 될 수 없으므로, TodoResponse의 id 프로퍼티를 String? 대신 String으로 선언하여 타입 안정성을 높이는 것이 좋습니다.
| let id: String? | |
| let id: String |
DevLog/Data/DTO/WebPageDTO.swift
Outdated
| struct WebPageResponse: Decodable { | ||
| @DocumentID var id: String? | ||
| struct WebPageResponse { | ||
| let id: String? |
There was a problem hiding this comment.
WebPageService의 makeResponse 함수를 보면 snapshot.documentID를 사용하여 id를 초기화하고 있습니다. documentID는 nil이 될 수 없으므로, WebPageResponse의 id 프로퍼티를 String? 대신 String으로 선언하여 타입 안정성을 높이는 것이 좋습니다.
| let id: String? | |
| let id: String |
|
/gemini review |
There was a problem hiding this comment.
Code Review
이번 PR은 데이터 레이어와 인프라 레이어 간의 의존성을 분리하여 아키텍처를 개선하는 중요한 리팩토링 작업을 담고 있습니다. DTO를 데이터 레이어로 이동시키고 Firebase 종속성을 제거하여, 각 레이어의 역할과 책임을 명확히 한 점이 인상적입니다. 특히, Firestore 데이터 파싱 로직을 서비스 내의 makeResponse 팩토리 메소드로 옮겨 Infra 레이어에 캡슐화한 것은 매우 좋은 접근 방식입니다. 전반적으로 코드의 유연성과 테스트 용이성이 크게 향상될 것으로 기대됩니다. 몇 가지 유지보수성 향상을 위한 제안을 리뷰 코멘트로 남겼으니 확인 부탁드립니다.
| private extension PushNotificationService { | ||
| func makeResponse(from snapshot: QueryDocumentSnapshot) -> PushNotificationResponse? { | ||
| let data = snapshot.data() | ||
| guard | ||
| let title = data["title"] as? String, | ||
| let body = data["body"] as? String, | ||
| let receivedAt = data["receivedAt"] as? Timestamp, | ||
| let isRead = data["isRead"] as? Bool, | ||
| let todoID = data["todoID"] as? String, | ||
| let todoKind = data["todoKind"] as? String else { | ||
| return nil | ||
| } | ||
|
|
||
| return PushNotificationResponse( | ||
| id: snapshot.documentID, | ||
| title: title, | ||
| body: body, | ||
| receivedAt: receivedAt.dateValue(), | ||
| isRead: isRead, | ||
| todoID: todoID, | ||
| todoKind: todoKind | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Firestore 필드 이름을 수동으로 파싱하는 로직을 추가하셨네요. 문자열 리터럴을 직접 사용하는 것은 오타에 취약하여 런타임 에러를 유발할 수 있습니다. 필드 이름을 enum으로 정의하여 상수로 관리하면 타입-세이프하게 접근할 수 있어 코드의 안정성과 유지보수성을 높일 수 있습니다.
private extension PushNotificationService {
private enum FieldKey: String {
case title, body, receivedAt, isRead, todoID, todoKind
}
func makeResponse(from snapshot: QueryDocumentSnapshot) -> PushNotificationResponse? {
let data = snapshot.data()
guard
let title = data[FieldKey.title.rawValue] as? String,
let body = data[FieldKey.body.rawValue] as? String,
let receivedAt = data[FieldKey.receivedAt.rawValue] as? Timestamp,
let isRead = data[FieldKey.isRead.rawValue] as? Bool,
let todoID = data[FieldKey.todoID.rawValue] as? String,
let todoKind = data[FieldKey.todoKind.rawValue] as? String else {
return nil
}
return PushNotificationResponse(
id: snapshot.documentID,
title: title,
body: body,
receivedAt: receivedAt.dateValue(),
isRead: isRead,
todoID: todoID,
todoKind: todoKind
)
}
}| private extension TodoService { | ||
| func makeResponse(from snapshot: QueryDocumentSnapshot) -> TodoResponse? { | ||
| makeResponse(documentID: snapshot.documentID, data: snapshot.data()) | ||
| } | ||
|
|
||
| func makeResponse(from snapshot: DocumentSnapshot) -> TodoResponse? { | ||
| guard let data = snapshot.data() else { | ||
| return nil | ||
| } | ||
| return makeResponse(documentID: snapshot.documentID, data: data) | ||
| } | ||
|
|
||
| func makeResponse(documentID: String, data: [String: Any]) -> TodoResponse? { | ||
| guard | ||
| let isPinned = data["isPinned"] as? Bool, | ||
| let isCompleted = data["isCompleted"] as? Bool, | ||
| let isChecked = data["isChecked"] as? Bool, | ||
| let title = data["title"] as? String, | ||
| let content = data["content"] as? String, | ||
| let createdAtTimestamp = data["createdAt"] as? Timestamp, | ||
| let updatedAtTimestamp = data["updatedAt"] as? Timestamp, | ||
| let tags = data["tags"] as? [String], | ||
| let kind = data["kind"] as? String else { | ||
| return nil | ||
| } | ||
|
|
||
| let dueDate = (data["dueDate"] as? Timestamp)?.dateValue() | ||
| return TodoResponse( | ||
| id: documentID, | ||
| isPinned: isPinned, | ||
| isCompleted: isCompleted, | ||
| isChecked: isChecked, | ||
| title: title, | ||
| content: content, | ||
| createdAt: createdAtTimestamp.dateValue(), | ||
| updatedAt: updatedAtTimestamp.dateValue(), | ||
| dueDate: dueDate, | ||
| tags: tags, | ||
| kind: kind | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Firestore 필드 이름을 수동으로 파싱하는 로직을 추가하셨네요. 문자열 리터럴을 직접 사용하는 것은 오타에 취약하여 런타임 에러를 유발할 수 있습니다. 필드 이름을 enum으로 정의하여 상수로 관리하면 타입-세이프하게 접근할 수 있어 코드의 안정성과 유지보수성을 높일 수 있습니다.
private extension TodoService {
private enum FieldKey: String {
case isPinned, isCompleted, isChecked, title, content, createdAt, updatedAt, dueDate, tags, kind
}
func makeResponse(from snapshot: QueryDocumentSnapshot) -> TodoResponse? {
makeResponse(documentID: snapshot.documentID, data: snapshot.data())
}
func makeResponse(from snapshot: DocumentSnapshot) -> TodoResponse? {
guard let data = snapshot.data() else {
return nil
}
return makeResponse(documentID: snapshot.documentID, data: data)
}
func makeResponse(documentID: String, data: [String: Any]) -> TodoResponse? {
guard
let isPinned = data[FieldKey.isPinned.rawValue] as? Bool,
let isCompleted = data[FieldKey.isCompleted.rawValue] as? Bool,
let isChecked = data[FieldKey.isChecked.rawValue] as? Bool,
let title = data[FieldKey.title.rawValue] as? String,
let content = data[FieldKey.content.rawValue] as? String,
let createdAtTimestamp = data[FieldKey.createdAt.rawValue] as? Timestamp,
let updatedAtTimestamp = data[FieldKey.updatedAt.rawValue] as? Timestamp,
let tags = data[FieldKey.tags.rawValue] as? [String],
let kind = data[FieldKey.kind.rawValue] as? String else {
return nil
}
let dueDate = (data[FieldKey.dueDate.rawValue] as? Timestamp)?.dateValue()
return TodoResponse(
id: documentID,
isPinned: isPinned,
isCompleted: isCompleted,
isChecked: isChecked,
title: title,
content: content,
createdAt: createdAtTimestamp.dateValue(),
updatedAt: updatedAtTimestamp.dateValue(),
dueDate: dueDate,
tags: tags,
kind: kind
)
}
}| private extension WebPageService { | ||
| func makeResponse(from snapshot: QueryDocumentSnapshot) -> WebPageResponse? { | ||
| let data = snapshot.data() | ||
| guard | ||
| let title = data["title"] as? String, | ||
| let url = data["url"] as? String, | ||
| let displayURL = data["displayURL"] as? String, | ||
| let imageURL = data["imageURL"] as? String else { | ||
| return nil | ||
| } | ||
|
|
||
| return WebPageResponse( | ||
| id: snapshot.documentID, | ||
| title: title, | ||
| url: url, | ||
| displayURL: displayURL, | ||
| imageURL: imageURL | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Firestore 필드 이름을 수동으로 파싱하는 로직을 추가하셨네요. 문자열 리터럴을 직접 사용하는 것은 오타에 취약하여 런타임 에러를 유발할 수 있습니다. 필드 이름을 enum으로 정의하여 상수로 관리하면 타입-세이프하게 접근할 수 있어 코드의 안정성과 유지보수성을 높일 수 있습니다.
private extension WebPageService {
private enum FieldKey: String {
case title, url, displayURL, imageURL
}
func makeResponse(from snapshot: QueryDocumentSnapshot) -> WebPageResponse? {
let data = snapshot.data()
guard
let title = data[FieldKey.title.rawValue] as? String,
let url = data[FieldKey.url.rawValue] as? String,
let displayURL = data[FieldKey.displayURL.rawValue] as? String,
let imageURL = data[FieldKey.imageURL.rawValue] as? String else {
return nil
}
return WebPageResponse(
id: snapshot.documentID,
title: title,
url: url,
displayURL: displayURL,
imageURL: imageURL
)
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
이 PR은 데이터와 인프라 레이어 간의 의존성을 줄이기 위해 DTO와 매퍼를 리팩터링하는 중요한 작업을 수행합니다. 전반적으로 변경 사항은 아키텍처를 개선하고 코드의 모듈성을 높이는 좋은 방향으로 진행되었습니다. DTO에서 FirebaseFirestore와 같은 특정 인프라 구현에 대한 의존성을 제거하고, Service 계층에서 수동으로 매핑하는 방식으로 변경한 것은 매우 훌륭한 결정입니다.
다만, Data 레이어에 남아있는 인프라 의존성 한 가지를 발견하여 리뷰 코멘트를 남겼습니다. 이 부분을 수정하면 PR의 목표를 더욱 완벽하게 달성할 수 있을 것입니다.
DevLog/Data/DTO/TodoDTO.swift
Outdated
|
|
||
| import Foundation | ||
|
|
||
| struct TodoRequest: Dictionaryable { |
There was a problem hiding this comment.
TodoRequest가 Dictionaryable 프로토콜을 채택하고 있는데, Dictionaryable의 구현이 FirebaseFirestore에 의존하고 있습니다. 이 PR의 목표가 데이터 레이어와 인프라 레이어를 분리하는 것인 만큼, 데이터 레이어의 DTO가 특정 인프라(Firebase) 기술에 의존하지 않도록 하는 것이 중요합니다.
Dictionaryable 프로토콜 채택을 제거하고, TodoRequest를 딕셔너리로 변환하는 로직은 Infra 레이어(예: TodoService) 내부에서 처리하는 것을 제안합니다. 이렇게 하면 데이터 레이어의 독립성을 유지할 수 있습니다.
예를 들어, TodoService.swift에 다음과 같은 private extension을 추가할 수 있습니다.
// In TodoService.swift
private extension TodoRequest {
func toDictionary() -> [String: Any] {
// Firestore.Encoder를 사용한 직렬화 로직
}
}| struct TodoRequest: Dictionaryable { | |
| struct TodoRequest { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
이 PR은 데이터 계층과 인프라 계층 간의 의존성을 분리하기 위한 중요한 리팩토링을 수행했습니다. DTO에서 Firebase 관련 코드를 제거하고, Infra 계층의 서비스 내에서 데이터 변환을 처리하도록 변경하여 아키텍처를 크게 개선했습니다. 이는 코드의 모듈성, 테스트 용이성 및 유지보수성을 향상시키는 좋은 변화입니다.
몇 가지 추가적인 개선을 위해 TodoRequest DTO에서 CodingKeys를 사용하여 인코딩을 더 명시적으로 제어하고, TodoService에서 Firestore.Encoder 설정을 통해 nil 값 처리를 자동화하는 방안을 제안했습니다. 전반적으로 매우 훌륭한 리팩토링입니다.
| struct TodoRequest: Encodable { | ||
| let id: String | ||
| let isPinned: Bool | ||
| let isCompleted: Bool | ||
| let isChecked: Bool | ||
| let title: String | ||
| let content: String | ||
| let createdAt: Date | ||
| let updatedAt: Date | ||
| let dueDate: Date? | ||
| let tags: [String] | ||
| let kind: TodoKind | ||
|
|
||
| } |
There was a problem hiding this comment.
TodoRequest를 인코딩할 때 id 프로퍼티가 인코딩에서 제외되도록 CodingKeys를 명시적으로 정의하는 것을 고려해 보세요. id는 Firestore 문서의 ID로 사용되며, 데이터 페이로드에는 포함되지 않는 것이 일반적입니다. CodingKeys를 사용하면 TodoService에서 수동으로 id 키를 제거하는 로직을 없앨 수 있어, DTO의 역할이 더 명확해지고 코드가 간결해집니다.
이 변경을 적용하려면 TodoService의 upsertTodo 메서드에서 data.removeValue(forKey: TodoFieldKey.id.rawValue) 라인도 함께 제거해야 합니다.
struct TodoRequest: Encodable {
let id: String
let isPinned: Bool
let isCompleted: Bool
let isChecked: Bool
let title: String
let content: String
let createdAt: Date
let updatedAt: Date
let dueDate: Date?
let tags: [String]
let kind: TodoKind
enum CodingKeys: String, CodingKey {
case isPinned, isCompleted, isChecked, title, content, createdAt, updatedAt, dueDate, tags, kind
}
}| if request.dueDate == nil { | ||
| data[TodoFieldKey.dueDate.rawValue] = NSNull() | ||
| } |
There was a problem hiding this comment.
dueDate가 nil일 때 NSNull()을 수동으로 할당하는 대신, Firestore.Encoder의 encodeNilValues 프로퍼티를 true로 설정하여 이 로직을 자동화하는 것을 고려해 보세요. 이렇게 하면 TodoRequest에 다른 옵셔널 프로퍼티가 추가되더라도 이 부분을 수정할 필요가 없어 유지보수성이 향상됩니다.
TodoService의 encoder 프로퍼티를 다음과 같이 수정할 수 있습니다:
private let encoder: Firestore.Encoder = {
let encoder = Firestore.Encoder()
encoder.encodeNilValues = true
return encoder
}()이 변경 후에는 이 if 문을 제거할 수 있습니다.
1 participant
