GoalAdd, GoalDetail 뷰컨트롤러 중복되는 코드 별도의 커스텀뷰로 이동, 목표 수정 기능 추가#116
GoalAdd, GoalDetail 뷰컨트롤러 중복되는 코드 별도의 커스텀뷰로 이동, 목표 수정 기능 추가#116
Conversation
…alFormView에서 받도록 처리
- 동일한 버튼을 터치할 때마다 저장하기와 수정하기가 번갈아가며 나타나도록 처리
| override func becomeFirstResponder() -> Bool { | ||
| super.becomeFirstResponder() | ||
| return textField.becomeFirstResponder() |
There was a problem hiding this comment.
GoalFormView에서 키보드를 올리기 위한 becomeFirstResponder을 호출하면,
자동으로 내부 titleField의 becomeFirstResponder()을 연이어 호출하기 위해 오버라이딩했습니다.
| private let formView = GoalFormView(activate: [.title, .subtitle, .period, .time]) | ||
|
|
There was a problem hiding this comment.
GoalAdd는 모든 필드에 대해 수정 가능해야 하기 때문에, GoalFormView를 초기화할 때 수정할 대상을 생성자의 파라미터로 받도록 했습니다.
| formView.rx.periodFieldTouched | ||
| .bind(to: viewModel.periodFieldTouched) | ||
| .disposed(by: disposeBag) | ||
|
|
||
| formView.rx.timeFieldTouched | ||
| .bind(to: viewModel.timeFieldTouched) | ||
| .disposed(by: disposeBag) | ||
|
|
There was a problem hiding this comment.
GoalFormView 내부의 InsertField 속성들을 모두 private으로 선언해서, 별도로 delegate proxy를 적용해서 periodFieldTouched, timeFieldTouched 속성을 정의하고 해당 이벤트 방출을 통해서만 기간 및 시간 필드 영역이 터치되었음을 알 수 있도록 했습니다.
| func updateGoalDetail(_ goal: Goal) -> Single<Result<Goal,GoalEditError>> { | ||
|
|
||
| .create { single in | ||
| do { | ||
| try realmManager.update(entity: goal.toEntity()) | ||
| single(.success(.success(goal))) | ||
| } catch { | ||
| single(.success(.failure(.goalEditFailure))) | ||
| } | ||
|
|
||
| return Disposables.create() | ||
| } | ||
| } |
There was a problem hiding this comment.
수정의 경우에는 추후 좀 더 정교하게 처리해야 될 것 같은데..
우선은 성공했을 경우에는 새로운 title, subtitle로 목표 정보를 바꾸고,
Realm에서의 작업이 실패하면 기존의 정보를 그대로 놔두고자 했습니다.
그래서 Repository에서 Single 이벤트를 방출할 때, 제네릭 타입으로 Result를 사용하도록 해봤습니다.
There was a problem hiding this comment.
앗 이 부분은 못 보고 지나쳤네요!
저 이번 과제전형에서도 데이터 영속화가 필요해서 Realm을 썼는데,
서버에서 이미지를 받아오면 Data타입을, 못 받아오면 nil을 반환하도록 하기 위해서 Repository에서 해당 메소드의 반환 타입을 Single<Data?> 타입을 반환하도록 했습니다!
그리고 Repository의 반환을 받는 바인딩 로직에서는 compactMap을 사용해서, nil을 반환 받을 경우에는 UIImage에 기존의 placeholder사진을 그대로 보여주도록 했었어요!
이 부분에서는 success가 두 번 겹치는 부분이 어색하다고 느껴져서 말씀을 드린건데, Repository의 반환을 받은 곳에서 분기해서 처리하기엔 Single<Result>타입이 더 가독성이 좋긴 하네요!
There was a problem hiding this comment.
아직 완전히 구현은 못했는데, 예외처리를 염두에 두고 Result로 받는 식으로 했어요! 보통 Alamofire이 URLSession을 감싸면서 응답을 전달할 때 Result로 전달하는데, 여기서 네트워크 처리에 대한 Error을 자체적으로 정의해서 Result에 담아서 전달하는 부분에 착안했어요.
나중에 말씀드리려 했는데, 이런식으로 DataSource에서 데이터를 받아올 때 네트워크 통신과 마찬가지로 Result로 데이터를 받도록 하고, 전역으로 저희가 한 번에 관리할 수 있는 Custom Error을 정의해두는 식으로 한 번 리팩토링 진행하면 직관적일 것 같아요!
예전에 서버개발할 때도 DB 혹은 다른 서버 모듈에서 데이터를 받아서 리턴하는 경우가 많은데, 이럴 때 예상 가능한 범위의 예외는 전역으로 따로 정의해서 로그를 찍던, 로그 처리 서버로 보내던 하는 식으로 관리했는데 여기서도 한 번 적용해보면 낫지 않을까 생각했습니다 ㅎㅎ
.success 가 두 번 겹치는건 일단 RealmManager 부분에서는 예외가 발생하건, 발생하지 않건 Single 리턴값이 무조건 onNext에 해당하는 success를 방출해야 하기 때문에 저렇게 했습니다!
catch 부분에서는 .failure 로 감싸도 되는데, 이러면 Repository에서 Single이 방출하는 값을 처리할 때 catch 혹은 catchAndReturn 같은걸로 RealmManager에서 던지는 예외를 처리하는 식으로 수정할 수도 있을 것 같네요!
| private let formView = GoalFormView(activate: [.nothing]) | ||
|
|
There was a problem hiding this comment.
GoalDetail의 경우에는 맨 처음에는 아무것도 수정할 수 없는 상태여야 하기 때문에 위와 같이 .nothing이라는 케이스만을 생성자의 파라미터로 넘기도록 했습니다.
GoalFormView 내부에서는 .nothing을 받으면 바로 모든 필드를 비활성화 처리하도록 해놨습니다.
| editingBarItem.rx.tap | ||
| .scan(false) { lastState, newState in !lastState } | ||
| .map { $0 } | ||
| .withUnretained(self) | ||
| .bind(onNext: { (viewController, enableEditing) in | ||
| if enableEditing { | ||
| viewController.editingBarItem.title = "저장하기" | ||
| viewController.formView.activateFields(for: [.title, .subtitle]) | ||
| viewController.formView.becomeFirstResponder() | ||
| } else { | ||
| viewController.editingBarItem.title = "수정하기" | ||
| viewController.formView.activateFields(for: [.nothing]) | ||
| viewModel.saveButtonTouched.accept(()) | ||
| } | ||
| }) | ||
| .disposed(by: disposeBag) | ||
|
|
There was a problem hiding this comment.
수정하기 버튼을 터치할 때마다 저장하기 -> 수정하기 순서로 버튼의 title이 번갈아가며 바뀌도록 했습니다.
이를 좀 더 직관적으로 처리하기 위해서 scan 오퍼레이터를 활용했습니다.
맨 처음값을 false로 두고, false일 때는 수정하기 상태로, true일 때는 저장하기 상태로 되도록 처리했고, 각각의 상태가 될 때마다 activateFields 호출을 통해 GoalFormView 내부에서 원하는 필드들이 활성화되도록 처리했습니다.
또한 becomeFirstResponder 호출을 통해, 저장하기 상태가 되면 바로 키보드가 올라옴으로써 수정 가능하도록 사용자가 알게 하고자 했습니다.
There was a problem hiding this comment.
이런 방법도 있었네요ㅎㅎ 한 번 실행해서 확인해볼게요!
|
|
||
| let goalEditInfo = Observable | ||
| .combineLatest(goalTitleInput, goalSubtitleInput) | ||
| .map { (title, subtitle) -> Goal in | ||
| Goal( | ||
| uuid: goal.uuid, | ||
| title: title, | ||
| subtitle: subtitle, | ||
| progress: goal.progress, | ||
| period: goal.period, | ||
| certTime: goal.certTime, | ||
| category: goal.category, | ||
| createdDate: goal.createdDate | ||
| ) | ||
| } | ||
| .share() | ||
|
|
||
| saveButtonTouched | ||
| .withLatestFrom(goalEditInfo) | ||
| .withUnretained(self) | ||
| .flatMapLatest { viewModel, goal in | ||
| viewModel.repository.updateGoalDetail(goal) | ||
| } | ||
| .withUnretained(self) | ||
| .bind(onNext: { viewModel, result in | ||
| switch result { | ||
| case .success(let goalEditInfo): | ||
| viewModel.goalTitleOutput.accept(goalEditInfo.title) | ||
| viewModel.goalSubtitleOutput.accept(goalEditInfo.subtitle) | ||
| case .failure(_): | ||
| viewModel.goalTitleOutput.accept(goal.title) | ||
| viewModel.goalSubtitleOutput.accept(goal.subtitle) | ||
| } | ||
| }) | ||
| .disposed(by: disposeBag) |
There was a problem hiding this comment.
뷰컨트롤러에서 title, subtitle에 대한 입력값이 바뀔 때마다 뷰모델에서 goalEditInfo 라는 지역변수에 할당된 값을 통해, 수정 대상이 될 값을 매번 업데이트 하도록 했습니다.
추후 saveButtonTouched 이벤트가 방출되면 repository로 정보 수정을 요청하게 되고, repository에서 방출한 Result 타입의 값에 따라(success or failure) 뷰모델의 아웃풋값이 기존의 값을 유지할 지, 새로운 값으로 변경될 지 결정되도록 처리했습니다.
| class RxGoalFormViewDelegateProxy: DelegateProxy<GoalFormView, GoalFormViewDelegate>, DelegateProxyType, GoalFormViewDelegate { | ||
|
|
||
| static func registerKnownImplementations() { | ||
| self.register { RxGoalFormViewDelegateProxy(parentObject: $0, delegateProxy: self) } | ||
| } | ||
|
|
||
| static func currentDelegate(for object: ParentObject) -> Delegate? { | ||
| object.delegate | ||
| } | ||
|
|
||
| static func setCurrentDelegate(_ delegate: Delegate?, to object: ParentObject) { | ||
| object.delegate = delegate | ||
| } | ||
| } |
There was a problem hiding this comment.
delegate proxy를 설정한 부분입니다.
GoalFormView에서 터치가 발생했을 때, GoalFormViewDelegate의 delegate 메소드 호출을 통해 어떤 필드가 터치되었는 지 알 수 있도록 했는데, 이를 rx와 함께 사용하기 위해 위와 같이 처리했습니다.
이럴 경우 delegate proxy에서 GoalFormViewDelegate를 채택하게 되고, 추후 별도의 rx 오퍼레이터를 정의해서 결과적으로 delegate 메소드를 rx 오퍼레이터를 통해 호출하고자 했습니다.
몇몇 자료들을 참고했는데 여기 에 나온 부분을 제일 많이 참고했어요!
There was a problem hiding this comment.
쉽지 않은 내용이네요,, 고생 많으셨어요 제드ㅎㅎ
| var delegate: DelegateProxy<GoalFormView, GoalFormViewDelegate> { | ||
| return RxGoalFormViewDelegateProxy.proxy(for: base) | ||
| } | ||
|
|
||
| var periodFieldTouched: Observable<Void> { | ||
| delegate.methodInvoked(#selector(GoalFormViewDelegate.periodFiledTouched)) | ||
| .map { _ in Void() } | ||
| } | ||
|
|
||
| var timeFieldTouched: Observable<Void> { | ||
| delegate.methodInvoked(#selector(GoalFormViewDelegate.timeFieldTouched)) | ||
| .map { _ in Void() } | ||
| } |
There was a problem hiding this comment.
delegate proxy를 설정한 후, 위의 periodFieldTouched, timeFiledTouched 를 통해 delegate 메소드가 호출됨을 알 수 있도록 처리했습니다.
뷰컨트롤러에서 GoalFormViewDelegate를 채택할 필요 없이 rx.periodFieldTouched, rx.periodFieldTouched 만 사용하도록 하고자 했어요!
| func activateFields(for targets: Set<ActivationTarget>) { | ||
| guard !targets.contains(.nothing) else { | ||
|
|
||
| titleField.isTextFieldEnabled = false | ||
| subtitleField.isTextFieldEnabled = false | ||
| periodField.isUserInteractionEnabled = false | ||
| timeField.isUserInteractionEnabled = false | ||
| return | ||
| } | ||
|
|
||
| for target in targets { | ||
| switch target { | ||
| case .title: | ||
| titleField.isTextFieldEnabled = true | ||
| case .subtitle: | ||
| subtitleField.isTextFieldEnabled = true | ||
| case .period: | ||
| periodField.isUserInteractionEnabled = true | ||
| case .time: | ||
| timeField.isUserInteractionEnabled = true | ||
| default: | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ActivationTarget이라는 열거형을 내부에 별도로 선언하고,
title, subtitle, period, time 4가지 중 사용자가 파라미터로 전달한 항목들에 맞춰 해당 InsertField 속성들을 활성화하고자 했습니다.
우선은.. 제일 단순하게 for문을 돌면서 매 케이스마다 살펴보도록 했는데 이 부분은 좀 더 고민해보면 깔끔하게 개선할 수 있을 것 같아요(좀 더 고민해볼께요 ㅎㅎ..)
There was a problem hiding this comment.
모든 insertField의 isTextFieldEnabled 프로퍼티가 초깃값으로 false를 가지고 있다면 첫 번째 guard 문을 제거할 수 있을 것 같아요!
그리고, for문은 어쩔 수 없을 것 같은데, switch문을 dictionary로 개선하면 아래처럼 바꿀 수 있을 거 같아요!
func activateFields(for targets: ActivationTarget...) {
let activationTargetsMap: [ActivationTarget: InsertField] = [
ActivationTarget.title: titleField,
ActivationTarget.subtitle: subtitleField,
ActivationTarget.period: periodField,
ActivationTarget.time: timeField
]
for target in targets {
activationMap[target]?.isTextFieldEnabled = true
}
}매개변수 부분도 ,Set으로 형변환 하는 과정이 또 필요할 것 같아서 그냥 여러 개 받는 매개변수 형식으로 변경해봤습니다ㅎㅎ
There was a problem hiding this comment.
이건 일단 .nothing 케이스가 전달되면 무조건 비활성화하고 early exit하려고 한건데, 작성하면서도 좀 더 간단하게 해야겠다고 생각이 들었던 부분이에요 ㅠ
제안해주신대로 수정해보고 동작 확인해볼께요 ㅎㅎ!
| addGestureRecognizer(UITapGestureRecognizer(target: self, action: #selector(formTapped(_:)))) | ||
| } | ||
|
|
||
| @objc private func formTapped(_ gesture: UITapGestureRecognizer) { | ||
| //탭된 부분의 좌표별로 다른 메소드 호출 | ||
| let tappedLocation = gesture.location(in: self) | ||
| let tappedX = tappedLocation.x | ||
| let tappedY = tappedLocation.y | ||
|
|
||
| if (periodField.frame.minX...periodField.frame.maxX) ~= tappedX && | ||
| (periodField.frame.minY...periodField.frame.maxY) ~= tappedY { | ||
| (delegate?.periodFiledTouched ?? {})() | ||
| return | ||
| } | ||
|
|
||
| if (timeField.frame.minX...timeField.frame.maxX) ~= tappedX && | ||
| (timeField.frame.minY...timeField.frame.maxY) ~= tappedY { | ||
| (delegate?.timeFieldTouched ?? {})() | ||
| return | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
기존과 달리 InsertField 속성을 모두 GoalFormView에 private하게 숨겨놨기 때문에,
RxGesture을 직접 사용할 수가 없어졌습니다...
그래서 생성자 안에서 UITapGestureRecognizer을 추가하고 나서, GoalFormView 자체에 탭 제스쳐(터치)가 들어오면, 터치된 위치를 내부에서 계싼해서 periodField, timeField 영역에 터치되면 각각 GoalFormDelegate의 periodFieldTouched, tileFieldTouched 메소드를 호출하도록 처리했습니다.
이후에는 delegate proxy를 통해 뷰컨트롤러에서 delegate를 채택할 필요 없이, rx를 통해 위의 메소드 호출을 대신 알 수 있도록 처리했어요
| @objc protocol GoalFormViewDelegate: AnyObject { | ||
|
|
||
| @objc optional func periodFiledTouched() | ||
| @objc optional func timeFieldTouched() | ||
| } |
There was a problem hiding this comment.
delegate proxy가 채택할 GoalFormViewDelegate입니다.
@objc optional 로 선언한 이유는, 위와 같이 해야 delegate proxy를 선언할 때 프록시 객체에서 강제로 delegate method의 구현을 명시할 필요가 없었기 때문입니다.
구현을 명시하지 않는게 개인적으로는 좀 더 깔끔한 패턴이 되지 않을까 생각했습니다.
이 부분은 좀 더 학습해야 하는데.. 일단은 optional로 하지 않고, delegate method를 구현할 경우에는 RxSwift 자체에서 알 수 없는 경고성 메시지를 띄워서 일단 저렇게 해놨습니다.
There was a problem hiding this comment.
어려운 부분이네요,,, 다음에 같이 할 때 한 번 보여주세요!ㅋㅋ
Hansolkkim
left a comment
There was a problem hiding this comment.
제드 안녕하세요!
한 주 동안 과제 전형하느라 이제서야 늦게 확인했어요ㅠㅠ 양해해주셔서 감사합니다!
이전에 저도 RxDelegateFroxy에 대해서 공부해보고 적용시키는 방법 생각해봐야겠다 생각만 했었는데, 제드 쓰신 글보고 많이 배웠습니다ㅎㅎ 고생많으셨어요!
먼저 2번 GoalFormView 내부에서 periodField, timeField에 대한 터치 이벤트 처리에서 말씀주신 의견에 대해 먼저 답변 드리자면, 이전에 코드스쿼드 당시에 리뷰 받았던 내용을 말씀드리고 싶어요!
뷰들을 단순히 묶어주기만 하는 ContainerView 개념의 View들의 프로퍼티들은 굳이 숨기지 않아도 괜찮다는 리뷰를 받았었어요!
그리고 조금 꼼수 같을 수도 있지만, periodField, timeField를 fileprivate 접근제어자로 설정하고, extension을 같은 소스코드 파일 내에서 처리해주면 어떨까하는 생각도 들었습니다ㅎㅎ
final class GoalFormView: UIView {
fileprivate let periodField: InsertField = {
...
}()
fileprivate let timeField: InsertField = {
...
}()
...
}
extension Reactive where Base == GoalFormView {
var periodFieldTouched: TapControlEvent {
base.periodField.rx.tapGesture()
}
var timeFieldTouched: TapControlEvent {
base.timeField.rx.tapGesture()
}
}이런 내용 같이 얘기해보면서 새로 적용해보면 좋을 것 같습니다ㅎㅎ
| func activateFields(for targets: Set<ActivationTarget>) { | ||
| guard !targets.contains(.nothing) else { | ||
|
|
||
| titleField.isTextFieldEnabled = false | ||
| subtitleField.isTextFieldEnabled = false | ||
| periodField.isUserInteractionEnabled = false | ||
| timeField.isUserInteractionEnabled = false | ||
| return | ||
| } | ||
|
|
||
| for target in targets { | ||
| switch target { | ||
| case .title: | ||
| titleField.isTextFieldEnabled = true | ||
| case .subtitle: | ||
| subtitleField.isTextFieldEnabled = true | ||
| case .period: | ||
| periodField.isUserInteractionEnabled = true | ||
| case .time: | ||
| timeField.isUserInteractionEnabled = true | ||
| default: | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
모든 insertField의 isTextFieldEnabled 프로퍼티가 초깃값으로 false를 가지고 있다면 첫 번째 guard 문을 제거할 수 있을 것 같아요!
그리고, for문은 어쩔 수 없을 것 같은데, switch문을 dictionary로 개선하면 아래처럼 바꿀 수 있을 거 같아요!
func activateFields(for targets: ActivationTarget...) {
let activationTargetsMap: [ActivationTarget: InsertField] = [
ActivationTarget.title: titleField,
ActivationTarget.subtitle: subtitleField,
ActivationTarget.period: periodField,
ActivationTarget.time: timeField
]
for target in targets {
activationMap[target]?.isTextFieldEnabled = true
}
}매개변수 부분도 ,Set으로 형변환 하는 과정이 또 필요할 것 같아서 그냥 여러 개 받는 매개변수 형식으로 변경해봤습니다ㅎㅎ
| @objc protocol GoalFormViewDelegate: AnyObject { | ||
|
|
||
| @objc optional func periodFiledTouched() | ||
| @objc optional func timeFieldTouched() | ||
| } |
There was a problem hiding this comment.
어려운 부분이네요,,, 다음에 같이 할 때 한 번 보여주세요!ㅋㅋ
| Observable.just("목표 기간을 설정하세요.") | ||
| .withUnretained(self) | ||
| .bind { viewController, text in | ||
| viewController.periodField.text = text | ||
| viewController.periodField.textColor = UIColor(hexRGB: "#A7A7A7") | ||
| } | ||
| .disposed(by: disposeBag) |
There was a problem hiding this comment.
이 부분 GoalAddView에서는 초깃값을 안보여주고 어떤 InsertField인지에 대한 설명이 필요해서 설정해뒀던 부분인데, 혹시 다른 방식으로 구현해주신건가요?!
There was a problem hiding this comment.
설명을 초깃값을 임의로 문자열을 아얘 text에 넣기보다 placeHolder로 표시하는게 나을 것 같아서 수정했어요!
위에 제거된 코드대로면 text를 초기화하고, 색상을 강제로 흰색으로 지정하는데 InsertField 안에 placeHolder 속성이 이미 있기 때문에 이걸 좀 더 활용하는게 나을 것 같아요!
현재 작업 브랜치에서는 기간이랑 시간 부분 필드는 초깃값을 한 번 방출하고 있어서 placeHolder이 보이지 않을텐데, 나중에 초깃값 방출시키지 않고 화면 출력하면 제목이랑 설명 입력 부분과 동일하게 placeHolder이 출력되도록 하면 될듯합니다.
| editingBarItem.rx.tap | ||
| .scan(false) { lastState, newState in !lastState } | ||
| .map { $0 } | ||
| .withUnretained(self) | ||
| .bind(onNext: { (viewController, enableEditing) in | ||
| if enableEditing { | ||
| viewController.editingBarItem.title = "저장하기" | ||
| viewController.formView.activateFields(for: [.title, .subtitle]) | ||
| viewController.formView.becomeFirstResponder() | ||
| } else { | ||
| viewController.editingBarItem.title = "수정하기" | ||
| viewController.formView.activateFields(for: [.nothing]) | ||
| viewModel.saveButtonTouched.accept(()) | ||
| } | ||
| }) | ||
| .disposed(by: disposeBag) | ||
|
|
There was a problem hiding this comment.
이런 방법도 있었네요ㅎㅎ 한 번 실행해서 확인해볼게요!
| class RxGoalFormViewDelegateProxy: DelegateProxy<GoalFormView, GoalFormViewDelegate>, DelegateProxyType, GoalFormViewDelegate { | ||
|
|
||
| static func registerKnownImplementations() { | ||
| self.register { RxGoalFormViewDelegateProxy(parentObject: $0, delegateProxy: self) } | ||
| } | ||
|
|
||
| static func currentDelegate(for object: ParentObject) -> Delegate? { | ||
| object.delegate | ||
| } | ||
|
|
||
| static func setCurrentDelegate(_ delegate: Delegate?, to object: ParentObject) { | ||
| object.delegate = delegate | ||
| } | ||
| } |
There was a problem hiding this comment.
쉽지 않은 내용이네요,, 고생 많으셨어요 제드ㅎㅎ
앗 답변이 더 추가되었네요 ㅎㅎ 코멘트 모두 잘 읽었습니다! 위에 인용처리된 extension 언급하신 부분은, 일단 저희 프로젝트가 Reactive의 extension을 포함한 모든 extension을 별도의 디렉토리에 모아서 관리하고 있어서 저렇게 수정했었어요. 사실 언급해주신 방법과 동일한 맥락으로 tapGesture이 들어간 필드에 대해서만 get-only 수준으로 접근제어를 설정해서 사용했었는데, 이럴 경우에 tapGesture외에 다른 은닉되야 하는 속성에도 불필요하게 접근하게되서... 부득이하게 싹 다 private으로 하면서 조금 복잡해진 코드가 되버렸습니다.. 추가적으로 이번 작업 진행하면서 든 생각이기도 한데.. GoalAddViewController - GoalFormView 간의 의존관계를 두는 것이랑 GoalAddViewController - GoalFormView - InsertField 간의 의존관계를 두는 것의 차이를 생각해봤는데, 우선적으로 전자의 경우가 타입 하나를 수정했을 때 다른 객체에 영향이 가는 범위를 줄이지 않을까 생각했어요 ㅎㅎ 이 부분은 아직 좀 더 고민해보는중이라 나중에 만났을 때 같이 얘기해봐요 ㅎㅎ |
- entity로의 변환은 RealmManager 내부에서 처리
| func activateFields(for targets: Set<ActivationTarget>) { | ||
| func activateFields(for targets: Set<ActivationTarget>) { | ||
|
|
||
| var insertFieldMap: [ActivationTarget: InsertField] = [ | ||
| ActivationTarget.title: titleField, | ||
| ActivationTarget.subtitle: subtitleField, | ||
| ActivationTarget.period: periodField, | ||
| ActivationTarget.time: timeField | ||
| ] | ||
|
|
||
| guard !targets.contains(.nothing) else { | ||
|
|
||
| titleField.isTextFieldEnabled = false | ||
| subtitleField.isTextFieldEnabled = false | ||
| periodField.isUserInteractionEnabled = false | ||
| timeField.isUserInteractionEnabled = false | ||
| insertFieldMap.values.forEach { ActivationTarget.nothing.activate($0) } | ||
| return | ||
| } | ||
|
|
||
| for target in targets { | ||
| switch target { | ||
| case .title: | ||
| titleField.isTextFieldEnabled = true | ||
| case .subtitle: | ||
| subtitleField.isTextFieldEnabled = true | ||
| case .period: | ||
| periodField.isUserInteractionEnabled = true | ||
| case .time: | ||
| timeField.isUserInteractionEnabled = true | ||
| default: | ||
| return | ||
| } | ||
| } | ||
| targets.forEach { $0.activate(insertFieldMap[$0]) } |
There was a problem hiding this comment.
제안해주신대로 지역변수에 Dictionary를 하나 할당해서 개선했습니다 ㅎㅎ
guard문같은 경우에는, 수정하기의 경우 읽기모드 - 수정모드 - 읽기모드로의 전환이 빈번해서 맨 처음에 isTextFieldEnabled, isUserInteractionEnabled 값을 false로 설정해도 이걸 false - true - false ... 이런 식으로 매번 바꾸는 상황이 있어서 어쩔 수 없이 살렸습니다 ㅠㅠ
또 ActivationTarget 열거형 안에 아래와 같은 함수를 하나 만들어서 InsertField의 속성값을 변화시키는 부분을 함수 바깥으로 이동시켜서, 기존에 swtich문으로 인해 코드가 길어지던 부분을 개선해봤어요!
func activate(_ insertField: InsertField?) {
guard let insertField = insertField else { return }
switch self {
case .title, .subtitle:
insertField.isTextFieldEnabled = true
case .period, .time:
insertField.isUserInteractionEnabled = true
case .nothing:
insertField.isTextFieldEnabled = false
insertField.isUserInteractionEnabled = false
}
}There was a problem hiding this comment.
이전에 "피해야할 코딩 습관" 이라는 블로그 글에서 봤던 내용인데 사실 Dictionary를 사용해서 분기문을 해결할 수 있다는 건 알게 됐는데 어디서 쓸 수 있을지 애매하다고 느꼈는데, 이런 부분에서 사용하면 좋겠다 싶었습니다!
case 별로 실행 내용이 크게 다르지 않음에도 Switch 분기문을 사용하면 반복적으로 (비슷한) 코드를 나열해야하는 경우가 생기는데, 이런 반복을 Dictionary를 사용하면 제거할 수 있더라구요!
case별로 activate시에 사용할 프로퍼티 이름이 다른 부분이 있는데 이걸 열거형으로 숨겨준 것도 확실히 이해하기 좋게 개선된거 같습니다!👍
There was a problem hiding this comment.
오.. 글 공유 감사합니다 ㅋㅋ 저도 읽어봐야겠군요
딕셔너리 사용 제안해주신거 저도 좋아서 바로 적용했어요! 미리 한 번 key-value를 정의하고 나니까 뒤에 switch문 불필요하게 길게 쓸 필요 없어서 보기 훨씬 좋아진거 같아요 ㅎㅎ
| func update<T: Object>(entity: T) throws { | ||
| try? realm.write { | ||
| realm.add(entity, update: .modified) | ||
| func update<T: Persistable>(object: T) -> Single<T> { | ||
| let realm = realm | ||
| let entity = object.toEntity() | ||
|
|
||
| return Single.create { observer -> Disposable in | ||
|
|
||
| do { | ||
| try realm.write { | ||
| realm.add(entity, update: .modified) | ||
| observer(.success(object)) | ||
| } | ||
| } catch { | ||
| observer(.failure(error)) | ||
| } | ||
|
|
||
| return Disposables.create { } |
There was a problem hiding this comment.
RealmManager에 RxSwift 의존성을 추가하고, 기존의 update 함수가 Single 리턴값을 가지도록 수정했습니다.
이렇게 한 제일 큰 이유는 코멘트중에 Repository쪽에서 .success(.success(...)) 과 같이 success가 두 번 중첩되는 부분이 어색한 것을 개선해보려 했기 때문입니다!
앞의 success는 Single Observer이 리턴하는 enum case가 되고, 뒤의 success는 Reulst타입이 리턴하는 enum case가 되서 이를 분리시켜야 했기 때문에, 앞의 Single Observer의 enum case를 리턴하는 부분은 Repository가 아닌 RealmManager로 이동시켜봤어요..!
RxRealm 혹은 RxAlamofire도 보통 observer의 success에 응답을 담고, failure에는 에러를 담길래 비슷하게 처리해봤는데 이 부분은 어떻게 보시나요 혹시!!?
There was a problem hiding this comment.
우와 이런 방법이 있었군요,,
저는 그냥 어? 어색하네? 정도로만 끝냈는데 이렇게 방법을 찾아주시느라 고생하셨어요 제드,,ㅋㅋㅋ
확실히 가독성이 더 좋아진 것 같습니다!
| func updateGoalDetail(_ goal: Goal) -> Single<Result<Goal,Error>> { | ||
|
|
||
| .create { single in | ||
| do { | ||
| try realmManager.update(entity: goal.toEntity()) | ||
| single(.success(.success(goal))) | ||
| } catch { | ||
| single(.success(.failure(.goalEditFailure))) | ||
| } | ||
|
|
||
| return Disposables.create() | ||
| } | ||
| realmManager.update(object: goal) | ||
| .debug() | ||
| .map { _ in .success(goal) } | ||
| .catchAndReturn(.failure(GoalEditError.goalEditFailure)) |
There was a problem hiding this comment.
RealmManager의 update가 Single을 리턴함에 따라 기존의 Repository의 updateGoalDetail은, RealmManager의 리턴값을 받아서 그대로 새로운 이벤트를 방출시켜서 뷰모델로 전달하도록 흐름을 바꿔봤어요..!
개인적으로는 이렇게 하면 우선 .success 가 중첩되지 않을 뿐더러, Repository에서는 옵저버가 방출하는 값을 가지고 오로지 뷰모델에 넘겨주기 위한 Result값만을 다루면 되기 때문에 기존보다 좀 더 직관적이지 않을까.. 하는 생각이 들었어요.
There was a problem hiding this comment.
더 좋은 것 같습니다!
아 그리고 예전에 제가 코쿼때 리뷰로 받은 질문인데, Repository 객체를 Struct 타입으로 사용하신 이유가 있으신가요?
저는 그 때 답을 못 찾았습니닼ㅋㅋㅋㅋ
아 그리고 저기 debug() 코드가 제거 안된 것 같아요!ㅎㅎ
There was a problem hiding this comment.
저는 객체를 정의할 때 일단은 기본적으로 Struct로 만들고 이후에 추가적인 이유가 있으면 이걸 Class로 바꿔서 사용해요 ㅎㅎ
Struct를 기본으로 사용하는 이유는
- 성능상 Static Dispatch의 대상이 되기 때문에 기본적으로 Class보다 비용상 효율적
- 참조값이 아닌 값 자체를 주고받기 때문에 Thread-Safe 하다.(경쟁상태와 같은 것들을 고려할 필요X)
대충 이정도인데 여기서 만일
- 객체가 특정 '상태'를 가지면서, 2개 이상의 객체와 수시로 상태를 공유해야 할 경우, 다시 말해 참조값을 공유해야 할 경우
- 특정 프로토콜을 채택할 때, 기본적으로
AnyObject채택이 선언되있어서 클래스에만 적용이 가능한 경우 - 크게 Thread-Safety를 고려하지 않는 상황에서 Struct 타입의 인스턴스의 값복사가 지나치게 많아서 차라리 참조값을 넘기는게 값복사 비용보다 더 효율적인 경우
이런 상황이 예상되거나, 나중에 발생하면 Class로 타입을 정의하는 것 같아요..
보통은 클래스 사용 이유로 상속을 많이 들기도 하던데 Swift 자체는 상속도 괜찮지만 프로토콜 사용을 통해 상속이 가지는 대부분의 이점을 동일하게 살릴 수 있어서 상속 측면에서도 Class < Struct 인 상황이 많은 것 같아요
ViewModel같은 경우는 2개 이상의 View가 상태를 공유하는 경우가 있을 수 있어서 Class로 정의하는데, Repository 같은 경우에는 크게 객체 자체가 어떤 상태를 가지거나 공유하지 않고 함수들도 동일한 입력에 대해 항상 동일한 출력만을 보장하기 때문에 Struct로 주로 사용하는 것 같아요 ㅎㅎ
There was a problem hiding this comment.
오,, 감사합니다 제드!
"'상태'의 유무" 부분이 눈에 띄네요,,
사실 제가 사용한 모든 Repository들이 상태가 없는 경우가 많아서, 이런 부분 개선해보면 좋을 것 같아요ㅎㅎ 감사합니다!
close #103
작업 내용
이번 PR에서 작업한 내용은 크게 아래와 같습니다.
이슈 공유
1. GoalFormView를 생성한 이유
activateFields(for:)이라는 함수를 하나 구현해서, 원하는 InsertField를 선택적으로 사용자가 활성화할 수 있도록 했습니다.2. GoalFormView 내부에서 periodField, timeField에 대한 터치 이벤트 처리
rx.tapGesture()를 사용해서 터치 이벤트를 처리할 수 있었지만, GoalFormView가 추가되면서 해당 속성들이 private이라 직접알 수 없어서 바로 tapGesture를 사용할 수 없다는 문제가 있었습니다.rx.periodFieldTouched,rx.timeFieldTouched와 같은 식으로 오퍼레이터를 사용할 수 있고, delegate 함수가 호출될 때마다 해당 오퍼레이터들에 의해 이벤트가 자동 방출됩니다!