Skip to content
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

Usecase,Entities,FeedListRepository,FirebaseRemoteDataSource를 생성했습니다. #2

Closed
wants to merge 10 commits into from

Conversation

GeonH0
Copy link
Collaborator

@GeonH0 GeonH0 commented Jun 3, 2024

  • FeedItem이라는 entity를 정의하였습니다.
  • SceneDelegate에서 ViewController의 네이밍을 변경해주었습니다.
  • 피드를 가지고 오는 FetchFeedListUseCase와 피드를 검색할때 사용할 SearchFeedListUseCase를 정의했습니다.
  • firebase에서 데이터를 가지고오는 FirebaseRemoteDataSource 클래스내에, fetchFeedItems, searchFeedItems의 행동을 정의하였습니다.
  • FeedListRepository에서는 FirebaseRemoteDataSource에서 데이터를 가지고오는 클래스 입니다.
  • 완료한 부분의 다이어 그램입니다!
    스크린샷 2024-06-04 오전 12 27 19

Comment on lines 13 to 14
}
class FeedListRepositoryImpl: FeedListRepository {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개행 부탁드리구요, implement 클래스는 final로 선언하는게 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23aebe4 수정했습니다!

Comment on lines 28 to 29


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 개행인 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

23aebe4 수정했습니다!

import FirebaseFirestore

class FirebaseRemoteDataSource {
private let db = Firestore.firestore()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더 명확한 변수 네이밍이면 좋을 것 같아요.

return
}
guard let documents = querySnapshot?.documents else {
completion(.success([]))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

documents가 없어도 성공인건가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

성공은 했지만 빈배열일 경우를 생각해서 넣었습니다!

let data = doc.data()
guard
let title = data["title"] as? String,
let imageURL = data["imageURL"] as? [String] else {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

배열이 반환되는 거면 imageURLs 와 같이 복수형으로 네이밍하는게 좋을 것 같아요

let imageURL = data["imageURL"] as? [String] else {
return nil
}
return FeedItem(id: doc.documentID, title: title, imageURL: imageURL)
Copy link

@f-lab-barry f-lab-barry Jun 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문) document 모델은 어디서 정의하고 있나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 document는 파이어 베이스에서 제공하는 문서 자체의 ID를 선언했습니다. 하지만 해당 데이터의 id를 넣어야 할거 같아 수정하겠습니다!

Comment on lines 45 to 58
guard let documents = querySnapshot?.documents else {
completion(.success([]))
return
}
let feedItems = documents.compactMap { doc -> FeedItem? in
let data = doc.data()
guard
let title = data["title"] as? String,
let imageURL = data["imageURL"] as? [String] else {
return nil
}
return FeedItem(id: doc.documentID, title: title, imageURL: imageURL)
}
completion(.success(feedItems))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repository에서 받은 data를 entity로 변환하는 과정(도메인으로 변환하는 과정)을 거치는데,
따로 메서드로 분리하거나 entity 내에서 메서드를 별도로 갖는 것에 대해 어떻게 생각하세요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

적용시켜보겠습니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response 모델이 정의된 곳에 toDomain()
struct xxxResponse {
let title: String
let imageURLs: [String]
}

extension xxxResponse {
func toDomain() -> FeedItem {
return FeedItem(
title: title,
imageURLs: imageURLs.map { URL(string: $0) }
)
}
}


func toDomain() -> [FeedItem] { ... } // mapping data to domain

struct FeedItem {
let id: String
let title : String
let imageURL : [String]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let imageURL : [String]
let imageURLs : [String]

Comment on lines 37 to 40
db.collection("feedItems")
.whereField("title", isGreaterThanOrEqualTo: title)
.whereField("title", isLessThanOrEqualTo: "\(title)\u{f8ff}")
.getDocuments { (querySnapshot, error) in

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요게 제거가 되는게 맞나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isEqualTo를 사용하는것과 햇갈렸습니다 수정하겠습니다!



protocol SearchFeedListUseCase {
func execute(title: String, completion: @escaping (Result<[FeedItem], Error>) -> Void)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UseCase에서도 return 없이 Result 타입 completion 으로 처리하는 이유가 있을까요?
(비즈니스 로직 성공/실패는 어디서 핸들링하게 되는걸까요?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usecase에서도 FeedRlistepository에서 데이터를 가지고 받으면서 비동기 작업이 필요할거라 생각해서 completion으로 처리하였습니다!
비즈니스 로직 성공/실패는 viewmodel에서 핸들링 하려고 합니다!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • viewModel이 너무 많은 역할을 하게 되는 것, 뚱뚱해지는 것 방지
  • 비즈니스 로직 재사용성을 높임
  • SearchFeedListXXX에 대한 응집도 높임


import UIKit

class FeedListViewController : UIViewController {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final이 붙어도 좋을까요? (: 옆에 띄어쓰기도 신경써주세요~)

Suggested change
class FeedListViewController : UIViewController {
final class FeedListViewController: UIViewController {

@@ -8,10 +8,10 @@
import FirebaseFirestore

class FirebaseRemoteDataSource {
private let db = Firestore.firestore()
private let firebasedb = Firestore.firestore()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

firebaseDB

@f-lab-barry
Copy link

  • eedItem이라는 entity를 정의하였습니다.
  • SceneDelegate에서 ViewController의 네이밍을 변경해주었습니다.
  • 피드를 가지고 오는 FetchFeedListUseCase와 피드를 검색할때 사용할 SearchFeedListUseCase를 정의했습니다.
  • firebase에서 데이터를 가지고오는 FirebaseRemoteDataSource 클래스내에, fetchFeedItems, searchFeedItems의 행동을 정의하였습니다.
  • FeedListRepository에서는 FirebaseRemoteDataSource에서 데이터를 가지고오는 클래스 입니다.
  • 완료한 부분의 다이어 그램입니다!

=> 배경도 있으면 좋을 것 같아요: 이 작업을 왜 하게 됐는지. 설계 관련 배경이나 진행에 대한 내용도 좋음
=> 이 PR에서 변경된 내용에 대한 설명: 여기에 리뷰어가 알면 도움이 될만한 내용 (건호님이 고민했던 내용이 뭐였는지)

Copy link

@f-lab-barry f-lab-barry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수고하셨습니다. 방금 남긴 2건의 코멘트만 확인해서 수정하고 머지 하시면 될 것 같아요.

Comment on lines 8 to 12
struct FeedItem {
let id: String
let title : String
let imageURLs : [String]
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 새로 정의된 값 (Recipe)으로 업데이트만 부탁드릴게요.

@@ -19,7 +19,12 @@ class DefaultFetchFeedListUseCase: FetchFeedListUseCase {

func execute(completion: @escaping (Result<[FeedItem], Error>) -> Void) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FeedItem이 아닌 새로 정의한 도메인 Recipe로 받도록 변경해야할 것 같아요~

Copy link

sonarqubecloud bot commented Jun 8, 2024

@GeonH0 GeonH0 closed this Jun 9, 2024
@GeonH0 GeonH0 deleted the feature/feed branch September 12, 2024 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants