[Refactor/#289] Browser를 분리해 코드 가독성을 개선한다. #299
Conversation
YunDaeHyeon
requested review from
GRJeon,
opficdev and
sangYuLv
and removed request for
GRJeon and
sangYuLv
YunDaeHyeon
left a comment
YunDaeHyeon
left a comment
There was a problem hiding this comment.
📄 PR Code Review Report
🔍 Summary
이번 Pull Request는 Browser 클래스의 핵심 로직을 BrowsingManager, BrowserCommandManager, BrowserStreamManager와 같은 전용 매니저 클래스들로 성공적으로 분리하여 모듈화 수준을 크게 향상시켰습니다. 이는 코드의 가독성, 유지보수성, 그리고 테스트 용이성을 비약적으로 개선했습니다. 새로운 Swift Testing 프레임워크를 활용한 테스트 코드도 인상적입니다.
전반적으로 코드는 잘 작성되었으며 의도된 기능을 수행할 것으로 보입니다. 다만, .xcscheme 파일의 잠재적인 오타와 Browser 초기화 로직의 미묘한 순서 문제, 그리고 몇몇 테스트 케이스의 논리적 결함이 발견되어 생산 환경 배포 전 수정이 필요합니다.
⚠️ Key Issues
-
Xcode Scheme 파일 (
mirroringBoothTests.xcscheme)의LastUpgradeVersion오타 추정:LastUpgradeVersion = "2610"으로 설정되어 있습니다. 이는 현재 Xcode 버전인15.x또는 미래 버전의16.x와 비교했을 때 비정상적으로 높은 값입니다. 실제 사용 중인 Xcode 버전과 맞지 않을 경우 빌드 시스템에서 예상치 못한 문제가 발생할 수 있습니다.1610또는1510등의 오타일 가능성이 높으니 확인이 필요합니다. -
Browser초기화 시super.init()호출 순서 오류:Browser클래스의init메서드에서commandManager.delegate = self와mirroringHeartBeater.delegate = self를super.init()호출 이전에 설정하고 있습니다. Swift에서self는super.init()이 완료된 후에야 완전히 초기화된 것으로 간주됩니다.self를 delegate로 할당하기 전에super.init()이 먼저 호출되어야 합니다. 이는 런타임에 미묘한 문제를 일으킬 수 있으므로, delegate 할당 코드를super.init()호출 뒤로 옮겨야 합니다. -
BrowserTests.swift내 명령 수신 테스트의 논리적 결함:heartBeat_명령_수신시_HeartBeater가_beat를_호출한다알_수_없는_명령_수신시_크래시하지_않는다
이 두 테스트 케이스는browser.session(_:didReceive:fromPeer:)메서드를 직접 호출하며MCSession의 목(mock) 객체를 전달합니다. 그러나Browser내부의session(_:didReceive:fromPeer:)메서드는 수신된 세션이mirroringCommandSession또는remoteSession중 하나와 일치하는지 확인하는 조건문(if session === mirroringCommandSession || session === remoteSession)을 가지고 있습니다. 현재 테스트에서 전달되는 목 세션은 이 조건에 해당하지 않으므로,commandManager.execute(data: data)코드가 실제로 호출되지 않습니다. 결과적으로 이 테스트들은 의도된 명령 처리 로직을 검증하지 못하고 단순히 "크래시가 발생하지 않는다"는 것만을 확인합니다. 해당 테스트 케이스들이BrowserCommandManager로의 위임을 제대로 검증할 수 있도록 목 세션을Browser의 내부 세션 중 하나로 설정하거나,BrowserCommandManager의execute메서드가 호출되었는지 직접 검증하는 방식으로 수정이 필요합니다.
🛠 Improvement Suggestions
-
Browser의MirroringDeviceCommand및RemoteDeviceCommand의존성:BrowserCommandManager에서Advertiser.CameraDeviceCommand를 사용하여 명령을 파싱합니다. 이는Browser와Advertiser간의 명령어 정의에 대한 암묵적인 결합을 나타냅니다. 현재는 잘 작동하지만, 장기적으로는AdvertiserCommandManager와BrowserCommandManager가 공유하는 공통 명령 Enum을 정의하여 두 모듈 간의 결합도를 낮추는 것을 고려해볼 수 있습니다. (예:SharedCommandEnum) -
로그 가독성 개선: 일부 로그 메시지에 기기 ID나 연결된 세션 타입과 같은 중요한 정보가 포함되어 있지만, 모든 관련 정보가 일관적으로 포함되지는 않습니다. 예를 들어,
logger.warning("사진 전송 실패 : \(error.localizedDescription)")메시지에는 어느 기기로의 전송이 실패했는지와 같은 추가 정보가 있으면 디버깅에 더 유용할 것입니다.
✅ Positive Observations
-
성공적인 모듈화 및 책임 분리:
Browser클래스의 로직을BrowsingManager,BrowserCommandManager,BrowserStreamManager로 깔끔하게 분리한 점은 매우 훌륭합니다. 각 매니저가 명확한 책임을 가지게 되어 코드의 이해도를 높이고 잠재적인 버그 발생 가능성을 줄였습니다. -
AsyncStream및Continuation관리 개선:BrowserStreamManager를 도입하여 모든 비동기 스트림과 그에 대한Continuation을 한 곳에서 관리하도록 한 점은 비동기 이벤트 처리의 일관성과 안전성을 크게 향상시켰습니다.bufferingNewest(1)정책도 실시간 스트림 데이터에 적합한 현명한 선택입니다. -
안정적인 명령어 처리 로직:
BrowserCommandManager에서DispatchQueue.main.async { [weak self] in ... }를 사용하여 UI 업데이트 및 상태 변경을 메인 스레드에서 안전하게 처리하고, 순환 참조를 방지하기 위해[weak self]를 사용한 점은 모범적인 패턴입니다. -
Swift Testing 프레임워크의 적극적인 활용: 새롭게 도입된 Swift Testing 프레임워크를 사용하여 비동기 로직과 스트림 이벤트를 효과적으로 테스트한 점이 뛰어납니다.
EventCollector와HeartbeatEventCollector와 같은 액터 기반 테스트 헬퍼는 비동기 테스트의 견고함을 높여줍니다. -
명령어 Enum의 중앙 집중화:
Browser.MirroringDeviceCommand와 같이 중첩되어 있던 명령어 Enum들을BrowserManager/BrowserCommands.swift파일로 이동시켜 독립적인 전역 Enum으로 만든 점은 코드 탐색성과 재사용성을 개선합니다.
YunDaeHyeon
changed the base branch from
develop
to
feat/#288-browser-test-code
YunDaeHyeon
force-pushed
the
refactor/#289-browser-refactoring
branch
from
d9533b8 to
62baf66
Compare
opficdev
left a comment
opficdev
left a comment
There was a problem hiding this comment.
고생하셨습니다!
폴더링을 BrowserManager 대신 Manager로 하는건 별로일까요?
| static let advertiser = Logger(subsystem: subsystem, category: "Advertiser") | ||
| static let advertisingManager = Logger(subsystem: subsystem, category: "AdvertisingManager") | ||
| static let advertiserCommandmanager = Logger(subsystem: subsystem, category: "AdvertiserCommandmanager") | ||
|
|
There was a problem hiding this comment.
Advertiser랑 Browser 로거를 좀 구분짓고 싶어서 개행 하나 넣은게 잡힌 것 같네요..ㅎㅎ
|
하단 Streaming 폴더 하위에 CameraManager라는 폴더가 있어서 네이밍을 동일하게 맞추고 싶었습니다! 하지만.. 말씀해주신 부분을 생각해보니 현재 분리 된 파일들이 Browser 라는 폴더 하위에 위치하니.. Manager 라는 이름이 현재 상황에선 더 적합할 것 같네용 👀 바로 적용하겠습니다~ |
YunDaeHyeon
force-pushed
the
feat/#288-browser-test-code
branch
from
22aee42 to
c9cd211
Compare
There was a problem hiding this comment.
이거 파일 이름도 BrowsingEvents로 가능한가요?
- Browser는 계산 프로퍼티로 스트림을 노출 - HeartBeeater는 스트림 매니저의 yield 메서드 사용 - 테스트 코드 모두 통과로 동등성 확인 완료
- MCNearbyServiceBrowserDelegate 분리 - discoveredPeers 상태 관리 분리 - Browser는 browsingManager 인스턴스를 사용하도록 개선 - delegate extenstion 제거
- executeCommand 메서드를 CommandManager의 execute(data:)로 분리 - BrowserCommandDelegate Protocol 정의 - Browser에서 delegate 채택
- 기존 Browser.MirroringDeviceCommand, Browser.RemoteDeviceCommand 사용처에서 Browser. 만 제거 - BrowserManager 폴더 생성 및 관련 파일 이동
YunDaeHyeon
force-pushed
the
refactor/#289-browser-refactoring
branch
from
68f6835 to
85405e8
Compare
- BrowserEvents 이름 변경
4 participants
🔗 연관된 이슈
📝 작업 내용
📌 요약
Browser책임을 분리하여 코드 가독성과 유지보수성을 개선했습니다.🔍 상세
분리된 파일(기능)은 아래 표로 정리했습니다.
BrowserStreamManagerBrowsingManagerMCNearbyServiceBrowserDelegate,discoveredPeers상태 관리BrowserCommandManagerexecuteCommand로직,BrowserCommandDelegate프로토콜 정의주요 변경 사항은 아래와 같습니다.
Browser에서 AsyncStream 관리, 기기 탐색, 명령 처리 로직을 별도 매니저로 분리했습니다.BrowserStreamManagerBrowser는 각 매니저를 조율하는 역할로 단순화하고자 했습니다.위 작업으로 기존 500줄이 넘어가는 Browser 코드를 약 100줄 이상 감소시킬 수 있었습니다.
또한 #298 에서 작업한 테스트 코드를 토대로
1개의 파일 분리 후 테스트 실행 -> 통과 확인 후 실기기 테스트 진행 -> 문제 없으면 다른 파일 분리의 순서로 점진적 분리를 진행했습니다.💬 리뷰 노트
현재 Lint에서
file_length경고가 발생합니다 (400줄 제한, 현재 412줄).추가 분리를 고민했으나 아래와 같은 이유로 현 상태에서 마무리했습니다.
MCSessionDelegate를 별도 파일로 분리할 수 있지만, 세션 로직과 밀접하게 연결되어 있어 오히려 코드 흐름 파악이 어려워질 수 있을 것이라고 판단했고 더 분리하면다른 작업에 영향이 크게 있을 것으로 예상했습니다.