[iOS] Coordinator 패턴을 적용한 구조 구현 #135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SwiftyJunnos
requested changes
Nov 29, 2023
Comment on lines
10
to
16
| final class AppCoordinator: Coordinator, | ||
| HomeMapCoordinatorDelegate, | ||
| SpotCoordinatorDelegate, | ||
| RewindCoordinatorDelegate, | ||
| SettingCoordinatorDelegate, | ||
| SearchMusicCoordinatorDelegate, | ||
| SaveJourneyCoordinatorDelegate { |
There was a problem hiding this comment.
Delegate는 extension으로 빼고 MARK로 구분시켜 주세요..!
Comment on lines
55
to
66
| func navigateToSetting(coordinator: HomeMapCoordinator) { | ||
| let settingCoordinator = SettingCoordinator(navigationController: navigationController) | ||
| settingCoordinator.delegate = self | ||
|
|
||
| self.childCoordinators.append(settingCoordinator) | ||
| settingCoordinator.start() | ||
| } | ||
|
|
||
| func navigateToHomeMap(coordinator: SpotCoordinator) { | ||
| removeChildCoordinator(child: coordinator) | ||
| navigationController.popViewController(animated: true) | ||
| } |
There was a problem hiding this comment.
push랑 pop이 구분 없이 navigate라서 뭐가 뭔지 헷갈리는 것 같기도 합니다.
push, pop, present에 따라서 메서드 이름을 구분짓는건 어떨까요?
Comment on lines
96
to
98
| while navigationController.topViewController !== homeMapViewController { | ||
| navigationController.popViewController(animated: true) | ||
| } |
There was a problem hiding this comment.
while문은 좀 위험해보이는데 다른 방법은 없을까요?
물론 위의 로직 덕분에 루프가 도는 일은 없겠지만 그래도 뭔가 꺼림직하네요.
ViewController 스택에서 homeMapViewController를 찾고,
만약에 있다면 그보다 위에 쌓인 ViewController들을 pop하고
없다면 setViewController를 호출하는 건 어떨까요?
Comment on lines
89
to
94
| guard let homeMapViewController = navigationController.viewControllers.first(where: { | ||
| $0 is HomeMapViewController | ||
| }) else { | ||
| navigationController.setViewControllers([HomeMapViewController()], animated: true) | ||
| return | ||
| } |
There was a problem hiding this comment.
이 메서드 자체를 프로토콜에서 기본 구현같은 걸로 빼두면 편리할 것 같습니다!
Closed
2 tasks
SwiftyJunnos
changed the base branch from
iOS/epic/JourneyListScene
to
iOS/epic/MSCoordinator
SwiftyJunnos
approved these changes
Nov 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
❗ 배경
Coordinator 패턴을 적용하였습니다.
전체적인 구조는 App Coordinator를 통해 모든 작업을 관리를 합니다.
🔧 작업 내역
🧪 테스트 방법
현재는 임시적인 두 화면과 UI가 없는 View Controller들로 이루어진 채 구현을 진행했습니다.
각 ViewController는 디렉토리명에서 알 수 있듯이 temp이며 각 패키지를 import해와서 구현해주시면 됩니다.
📝 리뷰 노트
Coordinator의 구조는 다음과 같습니다.
모든 동작의 총괄은 App Coordinator를 통해서 진행됩니다.
고민 거리
어떤 Coordinator Level에서 관리를 해야 하는가?
App Coordinator를 통하지 않고 한 단계 안의 Coordinator 단계에서(ex.HomeMapCoordinator) 이동을 진행하려고 했으나 App Coordinator에서 childCoordinators를 관리하고 있고 새로운 Coordinator를 생성, 삭제한 후 ViewController를 만들어 이동시키는 동작이 있기에 App Coordinator가 관리를 할 수 있도록 구현하였습니다.
parentCoordinator의 필요성
기존에는 parentCoordinator와 childCoordinators를 두어 포함관계를 가졌으나 의존성이 깊어지다보니 delegate를 통해 그 결합도를 낮춰보았습니다. 그 과정에서 parent Coordinator는 사라지게 되었습니다.
그럼 childCoordinators 필요성은?
childCoordinators의 필요성에 의문이 생겼는데 이것은 coordinator가 할당 해제되는 이슈를 막기 위해 존재해야 한다고 합니다.
https://zeddios.medium.com/coordinator-pattern-bf4a1bc46930#:~:text=Q%20%3A%20childeCoordinators%EB%8A%94,%EA%B0%80%20%EC%83%9D%EC%84%B1%EB%90%9C%EB%8B%A4%EA%B3%A0%20%ED%95%A9%EB%8B%88%EB%8B%A4.
📸 스크린샷
해당 스크린샷은 임시적인 두 화면 간의 이동을 나타내는 영상입니다.