✨ 여러 코디네이터를 이용한 화면 전환 + 태그 카테고리 화면 + 태그 관리 화면 구현 #21
Conversation
- child coordinators: TagCoordinator, PhotoNoteCoordinator, SearchCoordinator #13
- add functions to make child coordinators and start scene with them - adopt UINavigationControllerDelegate and add didShow method #13
- when child coordinator is done with its work - in UINavigationControllerDelegate.didShow() #13
- navigate to: PhotoNoteList, SelectPhoto, WritePhotoNote, PhotoNote #13
- navigate to: SelectPeriod #13
- navigate to: TagManagement #13
- in PhotoNoteListViewController: navigateToTagCategory(), navigateToSelectPhoto() - in TagCategoryViewController: navigateToTagManagement(), navigateToPhotoNoteList() - in TagManagementViewController: navigateToTagCategory() #13
- PhotoNoteViewController, SelectPhotoViewController, WritePhotoNoteViewController, SelectPeriodViewController #13
- KeyColor #14
|
merge 후 계속 진행하겠습니다! |
dev-Lena
changed the title
There was a problem hiding this comment.
고생하셨습니다👍 중복 코드와 하드코딩을 줄이기 위한 노력이 눈에 띄네요. 코드를 보면서 생긴 궁금점이나 개인적인 생각을 남겨보았습니다. 100% 제 주관이 담긴 리뷰이니 참고만 해주세요.
view controller들의 rootView를 생성하는 각 커스텀 뷰 클래스(ex. LoginViewController의 LoginView)에 private extension으로 SubviewFactory 구조체를 사용하여서 static 메서드로 뷰를 만드는데 이 부분이 적절(?)할까요?(static 사용에 대해서)
저는 코드에 정답이 있다고 생각하기 보단, 작성한 코드에 대해 왜 그렇게 구현했는지 설명할 수 있고, 상대방을 설득할 수 있다면 된다고 생각해요. 뷰를 코드로 작성할 때, 각각의 뷰를 꾸며주는 코드를 메서드로 작성해서 호출하는 방법이나 클로저를 활용해 뷰를 생성하는 방법을 사용하지 않고, Factory 구조체를 만들어서 뷰를 생성하신 이유가 있나요?
화면의 헤더 부분 또는 스크롤 뷰는 공통적으로 들어가서 ContentView로 구현을 했는데
프로토콜을 사용하기는 했지만 상속에 더 초점이 맞춰져 있습니다. 프로토콜 지향적으로 구현할 수 있는 방법이 있을까요?
뷰를 꾸며주는 코드를 재사용하기위해 ContentView, ContentViewWithHeader, ScrollableContentViewWithHeader 라는 클래스들을 구현하셨는데 개인적으로는 너무 과하게 추상화된게 아닌가 생각이 듭니다. 이 구조를 구현하기위해 작성된 빈 메서드들도 어색한 것 같구요.
공통적으로 들어가는 뷰를 3단계로 나누지 말고 하나로 구현했으면 더 좋았을 것 같아요.
좀 더 개선할 수 있는 부분
사소한 부분이긴한데 상속받지 않는 클래스엔 final을 명시해주고, IBOutlet이나 objc 메서드에도 private으로 은닉화를 해주면 더 좋을 것 같아요. 다른 개발자들에게 구현의도를 명확하게 전달해줄 수 있고, 약간의 성능향상도 된다고 합니다.
Increasing Performance by Reducing Dynamic Dispatch
| } | ||
| } | ||
| self.appCoordinator?.start() | ||
| self.window?.makeKeyAndVisible() |
There was a problem hiding this comment.
self의 명시 여부가 불규칙적인데, 기준을 두고 한가지로 통일하는 것이 좋을 것 같아요.
There was a problem hiding this comment.
오 좋은 지적이네요 감사합니다. 🙌🏻
self를 유추할 수 있는 곳은 생략하고 바로 유추하기 어렵다고 생각되는 부분(ex. URLRequest extension 등)에는 명시하는 방향으로 수정해야겠네요!
|
|
||
| func addSubviews() { } | ||
| func setupLayout() { } | ||
| func configureContentView() { } |
There was a problem hiding this comment.
빈 메서드를 구현하고 상속받는 방식으로 구현하신 이유가 있나요?
There was a problem hiding this comment.
Representable프로토콜과ContentView클래스는 모든 화면에서 공통적으로 사용하는 뷰(HeaderView, ScrollView)를 위한 청사진을 정의해놓기 위해서 만들었습니다.- 상위 클래스에서 미리 메서드들을 만들어놓고 하위 클래스에서 필요한 메서드만 override로 재정의해서 사용하도록 구현했습니다.
- 하위 클래스 구현부에서 메서드를 정의할 때 메서드 이름에 통일성을 주고 싶어 미리 정의해 놓았습니다.
정리하자면, 프로토콜과 상위 클래스에서 공통으로 사용할 수 있는 뷰 클래스를 만들기 위해 미리 필요할 수 있는 메서드를 정의해놓고 하위 클래스에서는 이 중에서 필요한것만 override해서 사용하는 방식을 생각하고 구현을 한건데요.
🤔 혹시 메서드 구현부가 없는게 부자연스러운건지 궁금하네요!
There was a problem hiding this comment.
🤔 혹시 메서드 구현부가 없는게 부자연스러운건지 궁금하네요!
저는 프로토콜을 채택했음에도 프로토콜의 메서드를 비어있게 두는것이 어색하다고 생각하는데, 이건 제 생각일 뿐이니 다른분들의 의견도 들어보면 좋을 것 같아요!
There was a problem hiding this comment.
저도 Representable을 채택해 구현한 메서드의 구현부가 없는게 좀 부자연스럽게 느껴지네요. 하위 뷰들이 ContentView를 상속받는다면 Representable이라는 프로토콜이 없어도 되지 않나요? Representable의 역할이 조금 모호하게 느껴집니다
There was a problem hiding this comment.
코멘트에 답변을 하면서 느낀점인데 제가 상속과 프로토콜에 대해 좀 더 살펴볼 필요가 있겠네요. 추후 진행하면서 피드백 받은 부분을 개선해 나가도록 하겠습니다.
감사합니다 👍
| let searchCoordinator = SearchCoordinator(navigationController: navigationController) | ||
| searchCoordinator.parentCoordinator = self | ||
| childCoordinators.append(searchCoordinator) | ||
| searchCoordinator.start() |
There was a problem hiding this comment.
coordinator의 parentCoordinator 설정, childCoordinators에 append, start 호출까지 반복되는 코드가 있는데, 줄일 수 있을 것 같아요.
There was a problem hiding this comment.
오 맞아요! 안그래도 제네릭 메서드를 이용하면 따로 뺄까 했었어요. 지금 다시 시도를 해봤는데 원래
- 이전에는 Coordinator 프로토콜에 parentCoordinator 프로퍼티를 추가하려고 했는데 프로토콜에서는 weak 참조로 프로퍼티 선언이 안되더라구요. (retain cycle 때문에 parentCoordinator는 weak 참조로 선언해야 합니다.)
- 그런데 이번에 다시 수정하려고 보니 ChildCoordinator 클래스에서 선언하고 상속하는 방법으로 하니까 고민하고 있던 문제가 해결됐네요.
또치군 덕분쓰 👍🏻
| self.navigationController.pushViewController(tagManagementViewController, animated: true) | ||
| func childDidFinish(_ child: Coordinator?) { | ||
| for (index, coordinator) in childCoordinators.enumerated() { | ||
| if coordinator === child { |
There was a problem hiding this comment.
for문에 where로 조건을 걸어주는 방법도 있습니다.
| childCoordinators.remove(at: index) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
인자로 받은 Coordinator를 배열에서 삭제하는것이 의도라면 반복문을 사용하지 않고 removeAll(where:)을 활용해 한줄로 줄일 수 있을 것 같아요.
There was a problem hiding this comment.
오오오오 그렇군요 처음알았어요! removeAll(where:)을 사용하니까 코드가 한결 간결하네요!
| } | ||
|
|
||
| var className: String { | ||
| return type(of: self).className |
There was a problem hiding this comment.
Self.className 으로도 표현할 수 있을 것 같아요.
| } | ||
| } | ||
|
|
||
| extension NSObject: ClassNameProtocol {} |
There was a problem hiding this comment.
코드를 보니 셀에서만 사용하는 것 같은데, NSObject가 ClassNameProtocol 채택해야하는 이유가 있나요?
There was a problem hiding this comment.
현재에는 UITableViewCell, UICollectionViewCell, UICollectionReusableView에서 사용하고 있는데 추후 다른 곳에서 사용 가능성이 있을 수 있다고 생각해서 사용할 각각의 클래스나 UIView가 채택하게 하지 않고 NSObject가 ClassNameProtocol을 채택하게 했습니다.
| import UIKit | ||
|
|
||
| extension UIView { | ||
| class func loadFromNibNamed(nibNamed: String, bundle: Bundle? = nil) -> UIView? { |
There was a problem hiding this comment.
class func과 static func이 동일한 기능을 한다고 생각해서 class func으로 선언을 한건데요.
좀 더 찾아보니(Swift Language Guide - Type Methods과 difference between static func and class func in Swift 참고) 둘이 다른 점이 있네요.
이 메서드는 추후 override를 하지 않으니 static으로 변경(static func vs class func 성능 비교)하는게 좋겠네요!
|
|
||
| public init(wrappedValue: T) { | ||
| self.wrappedValue = wrappedValue | ||
| self.wrappedValue.translatesAutoresizingMaskIntoConstraints = false |
There was a problem hiding this comment.
UsesAutoLayout propertyWrapper가 AutoresizingMask 비활성화 정도만 해주고 있는데, 이 부분은 LayoutGuideCompatible 프로토콜에서 레이아웃을 잡기 전에 이미 비활성화를 해주고 있어서 현재로선 아무 역할도 해주지 않는 것 같아요.
There was a problem hiding this comment.
네 이 부분 LayoutGuideCompatible에서 AutoresizingMask 비활성화하는 부분을 빼는 것을 깜빡했네요!
LayoutGuideCompatible에서 AutoresizingMask 비활성화를 구현하니 LayoutGuideCompatible의 메서드를 사용하지 않는 뷰에도 translatesAutoresizingMaskIntoConstraints를 false로 만들어줘야할 때, 이미 적용된 곳과 아닌 곳이 헷갈려서 만들었습니다. LayoutGuideCompatible에서 AutoresizingMask 비활성화하는 부분을 빼고 UsesAutoLayout를 사용할 예정입니다!
| import UIKit | ||
|
|
||
| extension UIColor { | ||
| static let keyColor = UIColor(named: "KeyColor")! |
There was a problem hiding this comment.
"KeyColor" 라는 이름을 가진 색이 없으면 앱이 터질텐데 괜찮을까요?
There was a problem hiding this comment.
음... 일단 색상이 나오면 된다고 생각하고 !로 처리한 다음에 넘어간 부분이네요. 이 부분에 대해서 다시 생각해보니 이렇게 처리하는건 위험하네요.
UIColor(named:) 보다 Color Library를 사용해서 구현하는 것이 더 좋을 것 같습니다.
🤔 그리고 이전부터 궁금했는데,
UIColor에 색상 추가할 때 색상마다 static let으로 선언 하시나요 아니면 public class
로 만드시나요? UIColor 내부를 보니 open class를 이용하여 각 색상(white, red 등)을 정의하고 있어서요!
There was a problem hiding this comment.
UIColor를 extension 하지는 않고, Assets에 컬러칩 만들어서 씁니다
| } | ||
| } | ||
| self.appCoordinator?.start() | ||
| self.window?.makeKeyAndVisible() |
There was a problem hiding this comment.
오 좋은 지적이네요 감사합니다. 🙌🏻
self를 유추할 수 있는 곳은 생략하고 바로 유추하기 어렵다고 생각되는 부분(ex. URLRequest extension 등)에는 명시하는 방향으로 수정해야겠네요!
|
|
||
| func addSubviews() { } | ||
| func setupLayout() { } | ||
| func configureContentView() { } |
There was a problem hiding this comment.
Representable프로토콜과ContentView클래스는 모든 화면에서 공통적으로 사용하는 뷰(HeaderView, ScrollView)를 위한 청사진을 정의해놓기 위해서 만들었습니다.- 상위 클래스에서 미리 메서드들을 만들어놓고 하위 클래스에서 필요한 메서드만 override로 재정의해서 사용하도록 구현했습니다.
- 하위 클래스 구현부에서 메서드를 정의할 때 메서드 이름에 통일성을 주고 싶어 미리 정의해 놓았습니다.
정리하자면, 프로토콜과 상위 클래스에서 공통으로 사용할 수 있는 뷰 클래스를 만들기 위해 미리 필요할 수 있는 메서드를 정의해놓고 하위 클래스에서는 이 중에서 필요한것만 override해서 사용하는 방식을 생각하고 구현을 한건데요.
🤔 혹시 메서드 구현부가 없는게 부자연스러운건지 궁금하네요!
| let searchCoordinator = SearchCoordinator(navigationController: navigationController) | ||
| searchCoordinator.parentCoordinator = self | ||
| childCoordinators.append(searchCoordinator) | ||
| searchCoordinator.start() |
There was a problem hiding this comment.
오 맞아요! 안그래도 제네릭 메서드를 이용하면 따로 뺄까 했었어요. 지금 다시 시도를 해봤는데 원래
- 이전에는 Coordinator 프로토콜에 parentCoordinator 프로퍼티를 추가하려고 했는데 프로토콜에서는 weak 참조로 프로퍼티 선언이 안되더라구요. (retain cycle 때문에 parentCoordinator는 weak 참조로 선언해야 합니다.)
- 그런데 이번에 다시 수정하려고 보니 ChildCoordinator 클래스에서 선언하고 상속하는 방법으로 하니까 고민하고 있던 문제가 해결됐네요.
또치군 덕분쓰 👍🏻
| childCoordinators.remove(at: index) | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
오오오오 그렇군요 처음알았어요! removeAll(where:)을 사용하니까 코드가 한결 간결하네요!
|
|
||
| if navigationController.viewControllers.contains(fromViewController) { | ||
| return | ||
| } |
There was a problem hiding this comment.
그러게요! 둘이 합치니까 훨씬 간결하고 좋네요!
| import Foundation | ||
|
|
||
| extension Int { | ||
| static let first = 0 |
There was a problem hiding this comment.
아 .zero를 깜빡 잊고 있었습니다. 굳이 Int를 extension해서 정의해줄 필요가 없었네요!
| } | ||
| } | ||
|
|
||
| extension NSObject: ClassNameProtocol {} |
There was a problem hiding this comment.
현재에는 UITableViewCell, UICollectionViewCell, UICollectionReusableView에서 사용하고 있는데 추후 다른 곳에서 사용 가능성이 있을 수 있다고 생각해서 사용할 각각의 클래스나 UIView가 채택하게 하지 않고 NSObject가 ClassNameProtocol을 채택하게 했습니다.
| import UIKit | ||
|
|
||
| extension UIColor { | ||
| static let keyColor = UIColor(named: "KeyColor")! |
There was a problem hiding this comment.
음... 일단 색상이 나오면 된다고 생각하고 !로 처리한 다음에 넘어간 부분이네요. 이 부분에 대해서 다시 생각해보니 이렇게 처리하는건 위험하네요.
UIColor(named:) 보다 Color Library를 사용해서 구현하는 것이 더 좋을 것 같습니다.
🤔 그리고 이전부터 궁금했는데,
UIColor에 색상 추가할 때 색상마다 static let으로 선언 하시나요 아니면 public class
로 만드시나요? UIColor 내부를 보니 open class를 이용하여 각 색상(white, red 등)을 정의하고 있어서요!
| import UIKit | ||
|
|
||
| extension UIView { | ||
| class func loadFromNibNamed(nibNamed: String, bundle: Bundle? = nil) -> UIView? { |
There was a problem hiding this comment.
class func과 static func이 동일한 기능을 한다고 생각해서 class func으로 선언을 한건데요.
좀 더 찾아보니(Swift Language Guide - Type Methods과 difference between static func and class func in Swift 참고) 둘이 다른 점이 있네요.
이 메서드는 추후 override를 하지 않으니 static으로 변경(static func vs class func 성능 비교)하는게 좋겠네요!
|
|
||
| public init(wrappedValue: T) { | ||
| self.wrappedValue = wrappedValue | ||
| self.wrappedValue.translatesAutoresizingMaskIntoConstraints = false |
There was a problem hiding this comment.
네 이 부분 LayoutGuideCompatible에서 AutoresizingMask 비활성화하는 부분을 빼는 것을 깜빡했네요!
LayoutGuideCompatible에서 AutoresizingMask 비활성화를 구현하니 LayoutGuideCompatible의 메서드를 사용하지 않는 뷰에도 translatesAutoresizingMaskIntoConstraints를 false로 만들어줘야할 때, 이미 적용된 곳과 아닌 곳이 헷갈려서 만들었습니다. LayoutGuideCompatible에서 AutoresizingMask 비활성화하는 부분을 빼고 UsesAutoLayout를 사용할 예정입니다!
dev-Lena
changed the title
| private func configure () { | ||
| hideNavigationBar() | ||
| photoNoteListView.moveToTagCategoryButton.addTarget(self, action: #selector(navigateToTagCategory), for: .touchUpInside) | ||
| photoNoteListView.moveToSelectPhotoButton.addTarget(self, action: #selector(navigateToSelectPhoto), for: .touchUpInside) |
There was a problem hiding this comment.
-
은닉화의 이유
: 말씀하신대로 외부에서 속성을 바꾸지 못하게 하여 안전성을 보장하는 것이 은닉화의 가장 큰 이유이자 거의 유일한 이유라고 생각합니다. 캡슐화를 하지 않는다면 최근에 제가 작성했던 문서인데 여기 예제를 살펴보면 은닉화/캡슐화를 하지 않고 속성을 외부로 노출 시키는 경우, 로직상에는 문제가 없지만 런타임 시점에 생각하지 못한 사이드이펙트가 발생해 앱이 크래시 나는 등의 위험 요소를 내재하게 됩니다. 최대한 개발자가 예측 가능한 동작을 하도록 만들기 위해서 은닉화가 필요하다고 생각합니다. -
커스텀 뷰 생성시 선호 방법
: 서브 클래스를 생성할 수 없고, 뷰를 static 키워드를 붙일만큼 먼저 생성되어야 할 이유를 잘 찾지 못하여서 static 키워드는 뷰 관련하여 웬만하면 사용하지 않으려 합니다. 뷰 컨트롤러 단 한곳에서만 사용되고, 거기에 필요한 뷰 요소가 적다면 클로저로 만들어 사용하는 편이고 뷰가 다양한 곳에서 사용되어 재사용할 필요가 있거나, 뷰를 정의하는 부분이 너무 길어진다면 뷰컨트롤러가 헤비해지는 걸 막기 위해 커스텀 클래스를 만들어 사용합니다. -
static method vs computed property
computed property로 뷰를 만든다는게 잘 이해가 안가요
| } else { | ||
| sectionTitle = TagManagementTableViewConstant.archivedHashtagsSectionHeaderTitle | ||
| } | ||
| return sectionTitle |
There was a problem hiding this comment.
저도 삼항연산자 사용을 선호하는 편인데, 삼항 연산자를 사용해서 한줄(99자나 120자 등)안에 담길때만 사용하려고 합니다. 삼항연산자 사용으로 인해 코드가 옆으로 너무 길어지게 되면 그건 그거대로 가독성을 해치게 되더라구요
| static let zeroPointEightfive: CGFloat = 0.85 | ||
| static let zeroPointZerofive: CGFloat = 0.05 | ||
| static let twenty: CGFloat = 20 | ||
| static let ten: CGFloat = 10 |
There was a problem hiding this comment.
뷰를 그릴 때 간격 등을 지정하기 위해 사용하는 CGFloat 값은 여러번 쓰인다면 static let rectWidthRatio: CGFloat = 0.4 이런식으로 변수화 해서 쓰고 한번 쓰고 말 값들이면 리터럴로 씁니다
| import UIKit | ||
|
|
||
| extension UIColor { | ||
| static let keyColor = UIColor(named: "KeyColor")! |
There was a problem hiding this comment.
UIColor를 extension 하지는 않고, Assets에 컬러칩 만들어서 씁니다
궁금한 점이나 다른 분들의 의견이 궁금한 점은 코멘트에 🤔 이모지와 함께 기록해놨습니다!
구현 화면
- 태그 카테고리 상세 구현은 아직 진행중입니다.
- Go 버튼을 누르면 태그 카테고리 상세 구현은 아직 진행중입니다.
구현한 부분
집중한 점
@propertyWrapper사용하기질문
view controller들의 rootView를 생성하는 각 커스텀 뷰 클래스(ex. LoginViewController의 LoginView)에 private extension으로 SubviewFactory 구조체를 사용하여서 static 메서드로 뷰를 만드는데 이 부분이 적절(?)할까요?(static 사용에 대해서)
화면의 헤더 부분 또는 스크롤 뷰는 공통적으로 들어가서 ContentView로 구현을 했는데
프로토콜을 사용하기는 했지만 상속에 더 초점이 맞춰져 있습니다. 프로토콜 지향적으로 구현할 수 있는 방법이 있을까요?
좀 더 개선할 수 있는 부분
질문, 잘못된 부분 피드백 모두 환영합니다!
다음 스텝
다음에는 태그 카테고리, 태그 관리, 태그별 포토 노트 목록 화면에 view model을 사용하여 상세 구현을 완료할 예정입니다.
closes #3 #7 #9 #13 #14 #15 #16 #18 #19 #20