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

[Feature] 화이트보드 탐색 로직 구현 #90

Merged
merged 7 commits into from
Nov 19, 2024

Conversation

choijungp
Copy link
Collaborator

🌁 Background

주변에 생성된 화이트보드를 탐색하는 로직을 구현하였습니다.
Repository, UseCase, ViewModel에 관련 로직을 구현하였고 해당 PR 이후 UI 구현에 들어가겠습니다.

👩‍💻 Contents

  • WhiteboardDelegate 구현
  • WhiteboardRepository (+ Interface)에 탐색 로직 구현
  • WhiteboardUseCase (+ Interface)에 탐색 로직 구현
  • WhiteboardListViewModel 화이트보드 탐색 로직 구현

✅ Testing

NearbyNetwork 관련 테스트는 어떻게 진행할 수 있을까요 ... 🤔

📝 Review Note

WhiteboardRepository에서 nearbyNetwork의 delegate를 self로 채택한 후
화이트보드를 탐색한 후 어떻게 전달해줘야 하는지 감이 잘 오지 않았습니다. ..

그래서 WhiteboardDelegate를 만들어 delegate에 전달하는 방식으로 진행하였습니다.

public func nearbyNetwork(_ sender: any NearbyNetworkInterface, didFind connections: [NetworkConnection]) {
    let foundWhiteboards = connections.map { Whiteboard(name: $0.name) }
    delegate?.whiteboard(self, didFind: foundWhiteboards)
}

UseCase에서는 Publisher를 사용해서 ViewModel에 bind하여 탐색한 whiteboard 목록들을 바로 확인할 수 있도록 하였습니다.

호옥시 delegate와 Combine 외에 더 좋은 방법이 있다면 언제든지 .. 코멘트 부탁드립니다.
delegate의 delegate를 사용중이라 ... 약간 이게 맞는지 잘 모르겠는 느낌이 있었습니다. 🫨

📣 Related Issue

@choijungp choijungp self-assigned this Nov 18, 2024
@choijungp choijungp linked an issue Nov 18, 2024 that may be closed by this pull request
2 tasks
Copy link
Collaborator

@taipaise taipaise left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!!
프레임워크의 동작을 테스트하는게 크게 의미가 없을거 같아 NearbyNetwork 모듈 자체를 테스트 할 필요는 없지 않을까..합니다.
다만 NearbyNetworkInterface를 채택한 모킹객체를 만들어 repository들을 테스트하는 건 좋은 것 같습니다~!
레포지토리에서 탐색 후 도메인으로 넘겨주는 문제는 저번주에 딴이랑 함께 고민한 문제와 같은 문제인 것 같습니다. 저희는 컴바인이나 asyncstream을 활용을 고민했는데요, 조이가 delegate를 이용해 구현한 방식도 좋다고 생각합니다. 어쩌면 저희가 너무 복잡하게 생각한건 아닌가 싶네요!! 다른 분들의 의견을 기다립니다~!~!

Comment on lines 18 to 22
public protocol WhiteboardDelegate: AnyObject {
/// 주변 화이트보드를 찾았을 때 실행됩니다.
/// - Parameters:
/// - whiteboards: 탐색된 화이트보드 배열
func whiteboard(_ sender: WhiteboardRepositoryInterface, didFind whiteboards: [Whiteboard])
Copy link
Collaborator

Choose a reason for hiding this comment

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

WhiteboardRepository에서 위임을 해줄거라면, WhiteboardRepositoryDelegate는 어떨까요?
WhiteboardDelegate라는 네이밍은 WhiteBoard 라는 객체가 위임할 delegate라고 이해가 되는듯 합니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

동 !!! 좋습니다 !!

private let repository: WhiteboardRepositoryInterface
private var repository: WhiteboardRepositoryInterface

public private(set) var whiteboardListPublisher: AnyPublisher<[Whiteboard], Never>
Copy link
Collaborator

Choose a reason for hiding this comment

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

읽기 전용(get)으로 선언했기 때문에 public let으로 가져가도 좋을 것 같습니다!

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 whiteboardSubject: PassthroughSubject<Whiteboard, Never>
let output: Output
private let whiteboardSubject: PassthroughSubject<Whiteboard, Never>
private let whiteboardListSubject: CurrentValueSubject<[Whiteboard], Never>
Copy link
Collaborator

Choose a reason for hiding this comment

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

WhiteboardUseCase에서도 whiteboardListSubject를 관리하고 있기 때문에 ViewModel에서도 중복되는 데이터를 가지고 있을 것 같습니다!

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에서는 PassthroughSubject로 수정해보겠습니다 ~ 🫡

Copy link
Collaborator

Choose a reason for hiding this comment

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

viewModel과 UseCase의 역할이 일부분 섞여 있는 것 같습니다!

  1. viewModel과 UseCase에서 모두 WhiteBoard 데이터를 CurrentValueSubject 로 관리하고 있는 것 같습니다!
  2. UseCase에 화이트보드를 생성하는 메서드가 있는데요, 실제로 화이트 보드의 생성은 ViewModel에서 담당하는 것 같습니다. (아마 이 부분은 조이가 담당하지 않은 것 같습니다! @ekrud99 확인해주시면 감사하겠습니다!)
  3. 화이트보드 검색도 검색하는 UseCase가 방출하는 값만 받아 viewModel에서 처리해도 좋을 듯 합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

11월 18일 ...... 저녁 9시 30분쯤 파일럿츠 모여 관련 회의 진행하였습니다 ........
관련 회의록 📝

Copy link
Collaborator

Choose a reason for hiding this comment

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

유익한 시간이었읍니다.!.!.!

Copy link
Member

@eemdeeks eemdeeks left a comment

Choose a reason for hiding this comment

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

NearbyNetwork 모듈 테스트 부분은 딩동의 의견과 완벽히 동일합니다!!

탐색한 화이트보드를 전달해 주는 방법을 Combine을 사용하셨는데요, 딩동이 달아준 코멘트와 같이 Swift Concurrency와 Combine 두가지를 고민 중 저희는 asyncStream을 사용했습니다.

이 부분에 대해서는 각 레이어의 관계에 따라 통일하면 좋을 것으로 생각합니다!!
다같이 얘기해보고 통일 해보면 좋을 것 같네요 :)

따로 코드에 대한 리뷰를 달 것은 더 보이지 않아 바로 approve합니다!

- WhiteboardDelegate 네이밍 변경 (WhiteboardRepositoryDelegate)
- WhiteboardUseCase Subject PassthroughSubject로 변경
Copy link
Collaborator

@taipaise taipaise left a comment

Choose a reason for hiding this comment

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

오늘도 고생하셨습니다~!~!!~~!

/// 주변 화이트보드를 찾았을 때 실행됩니다.
/// - Parameters:
/// - whiteboards: 탐색된 화이트보드 배열
func whiteboard(_ sender: WhiteboardRepositoryInterface, didFind whiteboards: [Whiteboard])
Copy link
Collaborator

Choose a reason for hiding this comment

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

단순 네이밍 컨벤션입니다만, 아래와 같은 네이밍은 어떨까요??

Suggested change
func whiteboard(_ sender: WhiteboardRepositoryInterface, didFind whiteboards: [Whiteboard])
func whiteboardRepository(_ sender: WhiteboardRepositoryInterface, didFind whiteboards: [Whiteboard])

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 !!!! 세심하지 못했군요 ~ 좋습니다 ~~~ 바아로 수정 ~ 하겠습니다 !! 감사띵똥

public final class WhiteboardUseCase: WhiteboardUseCaseInterface {
private let repository: WhiteboardRepositoryInterface
private var repository: WhiteboardRepositoryInterface
Copy link
Collaborator

Choose a reason for hiding this comment

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

delegate를 설정하기 위해 repository를 var로 설정하셨군요!!

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
Collaborator

@ekrud99 ekrud99 left a comment

Choose a reason for hiding this comment

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

browsing 로직을 잘 짜주셨네요!

@@ -10,6 +10,7 @@ import Foundation

public final class WhiteboardRepository: WhiteboardRepositoryInterface {
private var nearbyNetwork: NearbyNetworkInterface
public weak var delegate: WhiteboardRepositoryDelegate?
Copy link
Collaborator

Choose a reason for hiding this comment

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

아까 혼신의 고민을 한 흔적이 여깄군요 조이..
고생하셨습니당 ㅋㅋㅋㅋ

Copy link
Collaborator

Choose a reason for hiding this comment

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

혼고조..

@@ -38,7 +43,8 @@ extension WhiteboardRepository: NearbyNetworkDelegate {
}

public func nearbyNetwork(_ sender: any NearbyNetworkInterface, didFind connections: [NetworkConnection]) {
// TODO: -
let foundWhiteboards = connections.map { Whiteboard(name: $0.name) }
delegate?.whiteboardRepository(self, didFind: foundWhiteboards)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

메인 화면의 whiteboard cell에서 참여자를 표시하려면, 여기서 discoveryInfo를 같이 전달해줘야 할 수도 있을거같습니다!

@choijungp choijungp merged commit 23f7103 into develop Nov 19, 2024
2 checks passed
@choijungp choijungp deleted the feature/SearchWhiteboard branch November 19, 2024 02:46
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.

[Feature] 화이트보드 목록 표시 구현 [Feature] 화이트보드 탐색 구현
4 participants