Skip to content

GoalAdd, GoalDetail 뷰컨트롤러 중복되는 코드 별도의 커스텀뷰로 이동, 목표 수정 기능 추가#116

Open
junu0516 wants to merge 24 commits intodevfrom
refactor/GoalAddNEdit
Open

GoalAdd, GoalDetail 뷰컨트롤러 중복되는 코드 별도의 커스텀뷰로 이동, 목표 수정 기능 추가#116
junu0516 wants to merge 24 commits intodevfrom
refactor/GoalAddNEdit

Conversation

@junu0516
Copy link
Copy Markdown
Collaborator

@junu0516 junu0516 commented Dec 18, 2022

close #103

작업 내용

이번 PR에서 작업한 내용은 크게 아래와 같습니다.

  • GoalDetailViewController, GoalAddViewController에서 스택뷰에 동일한 InsertField 속성들을 추가하던 코드를, GoalFormView라는 별도의 클래스를 만들어서 2개의 뷰컨트롤러가 재사용하도록 처리
  • GoalFormView 내부의 InsertField 속성들을 private으로 선언하고, 별도의 didSet 블록을 가지는 속성과 delegate proxy를 통해 기존의 GoalDetailViewController, GoalAddViewController과 바인딩
  • 수정한 바인딩 방식에 맞춰, GoalDetailViewController에서 Goal 인스턴스의 title, subtitle 속성을 수정하는 기능 구현
    • scan 오퍼레이터를 사용해봤고, ViewModel -> Repository로의 데이터 수정 전달은 기존의 CRUD 방식과 동일합니다!

이슈 공유

GoalFormView를 생성하고, delegate proxy를 구현했는데 해당 부분에 대한 배경설명이 필요할 것 같아서 좀 더 보충해봤어요!

1. GoalFormView를 생성한 이유

  • 기존에 말씀드린대로 원래는 GoalDetailViewController 이라는 하나의 뷰컨트롤러만 두고, GoalAddViewController을 없애고 GoalDetailViewController 자체를 재사용하고자 했습니다.
  • 하지만 좀 더 살펴보니 2개의 뷰컨트롤러는 동일한 형태의 InsertField 속성들을 가지는 것 외에, 나머지 서브 뷰나 뷰모델 등에 있어서는 전혀 달랐기 때문에 하나의 뷰컨트롤러를 온전하게 공유하는 것은 우선 힘들다고 판단했습니다.
  • 따라서 동일한 형태의 InsertField 속성을 반복해서 선언하는 것부터 우선 제거해야겠다고 생각해서 GoalFormView를 따로 만든 후, 해당 뷰를 2개의 뷰컨트롤러가 각각 원하는 방식으로 사용하도록 했습니다.
  • 여기서 GoalFormView를 사용할 때, GoalDetailViewController의 경우에는 사실상 '읽기모드'와 같은 형식이라서, 모든 InsertField를 비활성화해야되고(텍스트 필드 입력, 터치 이벤트 수신 등), GoalAddViewController은 반대로 '쓰기모드'와 같은 형식이므로 모든 InsertField를 활성화해야 합니다.
  • 이러한 차이를 주고자 GoalFormView내부에 activateFields(for:) 이라는 함수를 하나 구현해서, 원하는 InsertField를 선택적으로 사용자가 활성화할 수 있도록 했습니다.
    • ActivationTarget은 이를 위해 별도로 정의한 열거형입니다.
  func activateFields(for targets:  Set<ActivationTarget>) {
      //nothing을 받으면 무조건 모든 필드를 비활성화하고 리턴
      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
          }
      }
  }

2. GoalFormView 내부에서 periodField, timeField에 대한 터치 이벤트 처리

  • 기존에는 뷰컨트롤러가 periodField, timeField의 참조를 직접 가지고 있었기 때문에, 바로 rx.tapGesture() 를 사용해서 터치 이벤트를 처리할 수 있었지만, GoalFormView가 추가되면서 해당 속성들이 private이라 직접알 수 없어서 바로 tapGesture를 사용할 수 없다는 문제가 있었습니다.
  • 이를 위해 아래와 같은 방식으로 접근해서 구현했습니다.(좀 더 좋은 방식이 있으면 같이 논의해봐요..)
    • GoalFormView의 생성자에서 자체적으로 UITapGestureRecognizer을 추가
    • UITapGestureRecognizer에 대한 target 메소드(아래의 코드)에서, 터치가 발생했을 때 해당 터치가 periodField, timeField영역에서 발생했는 지를 판별
    • 원하는 영역에서 터치가 발생했다면 미리 선언해둔 GoalFormViewDelegate의 delegate 메소드를 호출하도록 처리
    • GoalFormViewDelegate를 채택한 뷰에서는, 위와 같은 방식으로 periodField, timeField 영역이 터치됨을 알 수 있도록 함
@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
    }
}
  • 이 때, GoalFormViewDelegate의 선언을 optional로 아래와 같이 했는데, 이유는 다음과 같습니다.
    • 결과론적이지만 delegate proxy의 사용을 염두에 두고 했는데, optional하지 않을 경우에는 프록시 객체에서 강제로 프로토콜을 채택한 후, 메소드를 정의해야하는데, 이를 피하고자 했습니다.
    • optional하지 않을 경우에는 extension에서 추상메소드를 구체화하면 되는데, 우선적으로는 Dynamic Dispatch, Static Dispatch 중 어느 것의 대상이 될 것이냐의 차이 외에는 큰 차이가 보이지 않았습니다.
    • Dispatch의 경우에는 성능상 큰 차이가 없을 것 같아서.. 일단은 최대한 깔끔하게 코드를 정리해보고 싶어서 optional하게 작성했습니다.
    • 추가로 그냥 프록시 객체에서 GoalFormViewDelegate를 채택할 경우에는 Rx 자체에서 알 수 없는 경고 메시지가 떴습니다.(이 부분은 좀 더 정확히 확인해보고 문제 공유할께요!)
@objc protocol GoalFormViewDelegate: AnyObject {
    
    @objc optional func periodFiledTouched()
    @objc optional func timeFieldTouched()
}
  • 위와 같이 접근한 후 프록시 객체와 Reactive의 extension은 아래와 같이 정의됩니다.
    • 이후 뷰컨트롤러에서 GoalFormView 속성에 대해 rx.periodFieldTouched, rx.timeFieldTouched 와 같은 식으로 오퍼레이터를 사용할 수 있고, delegate 함수가 호출될 때마다 해당 오퍼레이터들에 의해 이벤트가 자동 방출됩니다!
extension Reactive where Base == GoalFormView {
    //rx에서 가지는 delegate 객체(프록시 객체)
    var delegate: DelegateProxy<GoalFormView, GoalFormViewDelegate> {
        return RxGoalFormViewDelegateProxy.proxy(for: base)
    }
    
    //delegate 함수 호출에 대한 rx 오퍼레이터 정의
    var periodFieldTouched: Observable<Void> {
        delegate.methodInvoked(#selector(GoalFormViewDelegate.periodFiledTouched))
            .map { _ in Void() }
    }
    
    var timeFieldTouched: Observable<Void> {
        delegate.methodInvoked(#selector(GoalFormViewDelegate.timeFieldTouched))
            .map { _ in Void() }
    }
}

// 뷰컨트롤러 대신 GoalFormViewDelegate를 채택한 프록시 객체 정의
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
    }
}

- 동일한 버튼을 터치할 때마다 저장하기와 수정하기가 번갈아가며 나타나도록 처리
@junu0516 junu0516 self-assigned this Dec 18, 2022
Comment on lines +105 to +107
override func becomeFirstResponder() -> Bool {
super.becomeFirstResponder()
return textField.becomeFirstResponder()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

GoalFormView에서 키보드를 올리기 위한 becomeFirstResponder을 호출하면,
자동으로 내부 titleField의 becomeFirstResponder()을 연이어 호출하기 위해 오버라이딩했습니다.

Comment on lines +15 to 16
private let formView = GoalFormView(activate: [.title, .subtitle, .period, .time])

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

GoalAdd는 모든 필드에 대해 수정 가능해야 하기 때문에, GoalFormView를 초기화할 때 수정할 대상을 생성자의 파라미터로 받도록 했습니다.

Comment on lines +82 to +89
formView.rx.periodFieldTouched
.bind(to: viewModel.periodFieldTouched)
.disposed(by: disposeBag)

formView.rx.timeFieldTouched
.bind(to: viewModel.timeFieldTouched)
.disposed(by: disposeBag)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

GoalFormView 내부의 InsertField 속성들을 모두 private으로 선언해서, 별도로 delegate proxy를 적용해서 periodFieldTouched, timeFieldTouched 속성을 정의하고 해당 이벤트 방출을 통해서만 기간 및 시간 필드 영역이 터치되었음을 알 수 있도록 했습니다.

Comment on lines +17 to +29
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()
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

수정의 경우에는 추후 좀 더 정교하게 처리해야 될 것 같은데..
우선은 성공했을 경우에는 새로운 title, subtitle로 목표 정보를 바꾸고,
Realm에서의 작업이 실패하면 기존의 정보를 그대로 놔두고자 했습니다.

그래서 Repository에서 Single 이벤트를 방출할 때, 제네릭 타입으로 Result를 사용하도록 해봤습니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

앗 이 부분은 못 보고 지나쳤네요!
저 이번 과제전형에서도 데이터 영속화가 필요해서 Realm을 썼는데,
서버에서 이미지를 받아오면 Data타입을, 못 받아오면 nil을 반환하도록 하기 위해서 Repository에서 해당 메소드의 반환 타입을 Single<Data?> 타입을 반환하도록 했습니다!
그리고 Repository의 반환을 받는 바인딩 로직에서는 compactMap을 사용해서, nil을 반환 받을 경우에는 UIImage에 기존의 placeholder사진을 그대로 보여주도록 했었어요!

이 부분에서는 success가 두 번 겹치는 부분이 어색하다고 느껴져서 말씀을 드린건데, Repository의 반환을 받은 곳에서 분기해서 처리하기엔 Single<Result>타입이 더 가독성이 좋긴 하네요!

Copy link
Copy Markdown
Collaborator Author

@junu0516 junu0516 Dec 27, 2022

Choose a reason for hiding this comment

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

아직 완전히 구현은 못했는데, 예외처리를 염두에 두고 Result로 받는 식으로 했어요! 보통 Alamofire이 URLSession을 감싸면서 응답을 전달할 때 Result로 전달하는데, 여기서 네트워크 처리에 대한 Error을 자체적으로 정의해서 Result에 담아서 전달하는 부분에 착안했어요.

나중에 말씀드리려 했는데, 이런식으로 DataSource에서 데이터를 받아올 때 네트워크 통신과 마찬가지로 Result로 데이터를 받도록 하고, 전역으로 저희가 한 번에 관리할 수 있는 Custom Error을 정의해두는 식으로 한 번 리팩토링 진행하면 직관적일 것 같아요!

예전에 서버개발할 때도 DB 혹은 다른 서버 모듈에서 데이터를 받아서 리턴하는 경우가 많은데, 이럴 때 예상 가능한 범위의 예외는 전역으로 따로 정의해서 로그를 찍던, 로그 처리 서버로 보내던 하는 식으로 관리했는데 여기서도 한 번 적용해보면 낫지 않을까 생각했습니다 ㅎㅎ

.success 가 두 번 겹치는건 일단 RealmManager 부분에서는 예외가 발생하건, 발생하지 않건 Single 리턴값이 무조건 onNext에 해당하는 success를 방출해야 하기 때문에 저렇게 했습니다!
catch 부분에서는 .failure 로 감싸도 되는데, 이러면 Repository에서 Single이 방출하는 값을 처리할 때 catch 혹은 catchAndReturn 같은걸로 RealmManager에서 던지는 예외를 처리하는 식으로 수정할 수도 있을 것 같네요!

Comment on lines +8 to 9
private let formView = GoalFormView(activate: [.nothing])

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

GoalDetail의 경우에는 맨 처음에는 아무것도 수정할 수 없는 상태여야 하기 때문에 위와 같이 .nothing이라는 케이스만을 생성자의 파라미터로 넘기도록 했습니다.

GoalFormView 내부에서는 .nothing을 받으면 바로 모든 필드를 비활성화 처리하도록 해놨습니다.

Comment on lines +76 to +92
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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

수정하기 버튼을 터치할 때마다 저장하기 -> 수정하기 순서로 버튼의 title이 번갈아가며 바뀌도록 했습니다.
이를 좀 더 직관적으로 처리하기 위해서 scan 오퍼레이터를 활용했습니다.

맨 처음값을 false로 두고, false일 때는 수정하기 상태로, true일 때는 저장하기 상태로 되도록 처리했고, 각각의 상태가 될 때마다 activateFields 호출을 통해 GoalFormView 내부에서 원하는 필드들이 활성화되도록 처리했습니다.
또한 becomeFirstResponder 호출을 통해, 저장하기 상태가 되면 바로 키보드가 올라옴으로써 수정 가능하도록 사용자가 알게 하고자 했습니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이런 방법도 있었네요ㅎㅎ 한 번 실행해서 확인해볼게요!

Comment on lines +66 to +100

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)
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

뷰컨트롤러에서 title, subtitle에 대한 입력값이 바뀔 때마다 뷰모델에서 goalEditInfo 라는 지역변수에 할당된 값을 통해, 수정 대상이 될 값을 매번 업데이트 하도록 했습니다.

추후 saveButtonTouched 이벤트가 방출되면 repository로 정보 수정을 요청하게 되고, repository에서 방출한 Result 타입의 값에 따라(success or failure) 뷰모델의 아웃풋값이 기존의 값을 유지할 지, 새로운 값으로 변경될 지 결정되도록 처리했습니다.

Comment on lines +62 to +75
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
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

delegate proxy를 설정한 부분입니다.
GoalFormView에서 터치가 발생했을 때, GoalFormViewDelegate의 delegate 메소드 호출을 통해 어떤 필드가 터치되었는 지 알 수 있도록 했는데, 이를 rx와 함께 사용하기 위해 위와 같이 처리했습니다.

이럴 경우 delegate proxy에서 GoalFormViewDelegate를 채택하게 되고, 추후 별도의 rx 오퍼레이터를 정의해서 결과적으로 delegate 메소드를 rx 오퍼레이터를 통해 호출하고자 했습니다.
몇몇 자료들을 참고했는데 여기 에 나온 부분을 제일 많이 참고했어요!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

쉽지 않은 내용이네요,, 고생 많으셨어요 제드ㅎㅎ

Comment on lines +47 to +59
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() }
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

delegate proxy를 설정한 후, 위의 periodFieldTouched, timeFiledTouched 를 통해 delegate 메소드가 호출됨을 알 수 있도록 처리했습니다.

뷰컨트롤러에서 GoalFormViewDelegate를 채택할 필요 없이 rx.periodFieldTouched, rx.periodFieldTouched 만 사용하도록 하고자 했어요!

Comment on lines +111 to +135
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
}
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

ActivationTarget이라는 열거형을 내부에 별도로 선언하고,
title, subtitle, period, time 4가지 중 사용자가 파라미터로 전달한 항목들에 맞춰 해당 InsertField 속성들을 활성화하고자 했습니다.

우선은.. 제일 단순하게 for문을 돌면서 매 케이스마다 살펴보도록 했는데 이 부분은 좀 더 고민해보면 깔끔하게 개선할 수 있을 것 같아요(좀 더 고민해볼께요 ㅎㅎ..)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

모든 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으로 형변환 하는 과정이 또 필요할 것 같아서 그냥 여러 개 받는 매개변수 형식으로 변경해봤습니다ㅎㅎ

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

이건 일단 .nothing 케이스가 전달되면 무조건 비활성화하고 early exit하려고 한건데, 작성하면서도 좀 더 간단하게 해야겠다고 생각이 들었던 부분이에요 ㅠ

제안해주신대로 수정해보고 동작 확인해볼께요 ㅎㅎ!

Comment on lines +89 to +110
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
}
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

기존과 달리 InsertField 속성을 모두 GoalFormView에 private하게 숨겨놨기 때문에,
RxGesture을 직접 사용할 수가 없어졌습니다...

그래서 생성자 안에서 UITapGestureRecognizer을 추가하고 나서, GoalFormView 자체에 탭 제스쳐(터치)가 들어오면, 터치된 위치를 내부에서 계싼해서 periodField, timeField 영역에 터치되면 각각 GoalFormDelegate의 periodFieldTouched, tileFieldTouched 메소드를 호출하도록 처리했습니다.

이후에는 delegate proxy를 통해 뷰컨트롤러에서 delegate를 채택할 필요 없이, rx를 통해 위의 메소드 호출을 대신 알 수 있도록 처리했어요

Comment on lines +176 to +180
@objc protocol GoalFormViewDelegate: AnyObject {

@objc optional func periodFiledTouched()
@objc optional func timeFieldTouched()
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

delegate proxy가 채택할 GoalFormViewDelegate입니다.

@objc optional 로 선언한 이유는, 위와 같이 해야 delegate proxy를 선언할 때 프록시 객체에서 강제로 delegate method의 구현을 명시할 필요가 없었기 때문입니다.

구현을 명시하지 않는게 개인적으로는 좀 더 깔끔한 패턴이 되지 않을까 생각했습니다.

이 부분은 좀 더 학습해야 하는데.. 일단은 optional로 하지 않고, delegate method를 구현할 경우에는 RxSwift 자체에서 알 수 없는 경고성 메시지를 띄워서 일단 저렇게 해놨습니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

어려운 부분이네요,,, 다음에 같이 할 때 한 번 보여주세요!ㅋㅋ

Copy link
Copy Markdown
Collaborator

@Hansolkkim Hansolkkim left a comment

Choose a reason for hiding this comment

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

제드 안녕하세요!
한 주 동안 과제 전형하느라 이제서야 늦게 확인했어요ㅠㅠ 양해해주셔서 감사합니다!

이전에 저도 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()
    }
}

이런 내용 같이 얘기해보면서 새로 적용해보면 좋을 것 같습니다ㅎㅎ

Comment on lines +111 to +135
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
}
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

모든 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으로 형변환 하는 과정이 또 필요할 것 같아서 그냥 여러 개 받는 매개변수 형식으로 변경해봤습니다ㅎㅎ

Comment on lines +176 to +180
@objc protocol GoalFormViewDelegate: AnyObject {

@objc optional func periodFiledTouched()
@objc optional func timeFieldTouched()
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

어려운 부분이네요,,, 다음에 같이 할 때 한 번 보여주세요!ㅋㅋ

Comment on lines -129 to -135
Observable.just("목표 기간을 설정하세요.")
.withUnretained(self)
.bind { viewController, text in
viewController.periodField.text = text
viewController.periodField.textColor = UIColor(hexRGB: "#A7A7A7")
}
.disposed(by: disposeBag)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이 부분 GoalAddView에서는 초깃값을 안보여주고 어떤 InsertField인지에 대한 설명이 필요해서 설정해뒀던 부분인데, 혹시 다른 방식으로 구현해주신건가요?!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

설명을 초깃값을 임의로 문자열을 아얘 text에 넣기보다 placeHolder로 표시하는게 나을 것 같아서 수정했어요!
위에 제거된 코드대로면 text를 초기화하고, 색상을 강제로 흰색으로 지정하는데 InsertField 안에 placeHolder 속성이 이미 있기 때문에 이걸 좀 더 활용하는게 나을 것 같아요!

현재 작업 브랜치에서는 기간이랑 시간 부분 필드는 초깃값을 한 번 방출하고 있어서 placeHolder이 보이지 않을텐데, 나중에 초깃값 방출시키지 않고 화면 출력하면 제목이랑 설명 입력 부분과 동일하게 placeHolder이 출력되도록 하면 될듯합니다.

Comment on lines +76 to +92
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이런 방법도 있었네요ㅎㅎ 한 번 실행해서 확인해볼게요!

Comment on lines +62 to +75
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
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

쉽지 않은 내용이네요,, 고생 많으셨어요 제드ㅎㅎ

@junu0516
Copy link
Copy Markdown
Collaborator Author

제드 안녕하세요! 한 주 동안 과제 전형하느라 이제서야 늦게 확인했어요ㅠㅠ 양해해주셔서 감사합니다!

이전에 저도 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()
    }
}

이런 내용 같이 얘기해보면서 새로 적용해보면 좋을 것 같습니다ㅎㅎ

@Hansolkkim

앗 답변이 더 추가되었네요 ㅎㅎ 코멘트 모두 잘 읽었습니다!
(제안해주신 코드 보고 좀 수정해서 커밋 푸시하고 머지할께요!)

위에 인용처리된 extension 언급하신 부분은, 일단 저희 프로젝트가 Reactive의 extension을 포함한 모든 extension을 별도의 디렉토리에 모아서 관리하고 있어서 저렇게 수정했었어요.

사실 언급해주신 방법과 동일한 맥락으로 tapGesture이 들어간 필드에 대해서만 get-only 수준으로 접근제어를 설정해서 사용했었는데, 이럴 경우에 tapGesture외에 다른 은닉되야 하는 속성에도 불필요하게 접근하게되서... 부득이하게 싹 다 private으로 하면서 조금 복잡해진 코드가 되버렸습니다..

추가적으로 이번 작업 진행하면서 든 생각이기도 한데.. GoalAddViewController - GoalFormView 간의 의존관계를 두는 것이랑 GoalAddViewController - GoalFormView - InsertField 간의 의존관계를 두는 것의 차이를 생각해봤는데, 우선적으로 전자의 경우가 타입 하나를 수정했을 때 다른 객체에 영향이 가는 범위를 줄이지 않을까 생각했어요 ㅎㅎ 이 부분은 아직 좀 더 고민해보는중이라 나중에 만났을 때 같이 얘기해봐요 ㅎㅎ

Comment on lines +111 to +125
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]) }
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

제안해주신대로 지역변수에 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
            }
        }

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

이전에 "피해야할 코딩 습관" 이라는 블로그 글에서 봤던 내용인데 사실 Dictionary를 사용해서 분기문을 해결할 수 있다는 건 알게 됐는데 어디서 쓸 수 있을지 애매하다고 느꼈는데, 이런 부분에서 사용하면 좋겠다 싶었습니다!

case 별로 실행 내용이 크게 다르지 않음에도 Switch 분기문을 사용하면 반복적으로 (비슷한) 코드를 나열해야하는 경우가 생기는데, 이런 반복을 Dictionary를 사용하면 제거할 수 있더라구요!

case별로 activate시에 사용할 프로퍼티 이름이 다른 부분이 있는데 이걸 열거형으로 숨겨준 것도 확실히 이해하기 좋게 개선된거 같습니다!👍

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

오.. 글 공유 감사합니다 ㅋㅋ 저도 읽어봐야겠군요
딕셔너리 사용 제안해주신거 저도 좋아서 바로 적용했어요! 미리 한 번 key-value를 정의하고 나니까 뒤에 switch문 불필요하게 길게 쓸 필요 없어서 보기 훨씬 좋아진거 같아요 ㅎㅎ

Comment on lines -27 to +43
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 { }
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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에는 에러를 담길래 비슷하게 처리해봤는데 이 부분은 어떻게 보시나요 혹시!!?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

우와 이런 방법이 있었군요,,
저는 그냥 어? 어색하네? 정도로만 끝냈는데 이렇게 방법을 찾아주시느라 고생하셨어요 제드,,ㅋㅋㅋ

확실히 가독성이 더 좋아진 것 같습니다!

Comment on lines +17 to +22
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))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

RealmManager의 update가 Single을 리턴함에 따라 기존의 Repository의 updateGoalDetail은, RealmManager의 리턴값을 받아서 그대로 새로운 이벤트를 방출시켜서 뷰모델로 전달하도록 흐름을 바꿔봤어요..!

개인적으로는 이렇게 하면 우선 .success 가 중첩되지 않을 뿐더러, Repository에서는 옵저버가 방출하는 값을 가지고 오로지 뷰모델에 넘겨주기 위한 Result값만을 다루면 되기 때문에 기존보다 좀 더 직관적이지 않을까.. 하는 생각이 들었어요.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

더 좋은 것 같습니다!

아 그리고 예전에 제가 코쿼때 리뷰로 받은 질문인데, Repository 객체를 Struct 타입으로 사용하신 이유가 있으신가요?
저는 그 때 답을 못 찾았습니닼ㅋㅋㅋㅋ

아 그리고 저기 debug() 코드가 제거 안된 것 같아요!ㅎㅎ

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

저는 객체를 정의할 때 일단은 기본적으로 Struct로 만들고 이후에 추가적인 이유가 있으면 이걸 Class로 바꿔서 사용해요 ㅎㅎ

Struct를 기본으로 사용하는 이유는

  • 성능상 Static Dispatch의 대상이 되기 때문에 기본적으로 Class보다 비용상 효율적
  • 참조값이 아닌 값 자체를 주고받기 때문에 Thread-Safe 하다.(경쟁상태와 같은 것들을 고려할 필요X)

대충 이정도인데 여기서 만일

  • 객체가 특정 '상태'를 가지면서, 2개 이상의 객체와 수시로 상태를 공유해야 할 경우, 다시 말해 참조값을 공유해야 할 경우
  • 특정 프로토콜을 채택할 때, 기본적으로 AnyObject 채택이 선언되있어서 클래스에만 적용이 가능한 경우
  • 크게 Thread-Safety를 고려하지 않는 상황에서 Struct 타입의 인스턴스의 값복사가 지나치게 많아서 차라리 참조값을 넘기는게 값복사 비용보다 더 효율적인 경우

이런 상황이 예상되거나, 나중에 발생하면 Class로 타입을 정의하는 것 같아요..

보통은 클래스 사용 이유로 상속을 많이 들기도 하던데 Swift 자체는 상속도 괜찮지만 프로토콜 사용을 통해 상속이 가지는 대부분의 이점을 동일하게 살릴 수 있어서 상속 측면에서도 Class < Struct 인 상황이 많은 것 같아요

ViewModel같은 경우는 2개 이상의 View가 상태를 공유하는 경우가 있을 수 있어서 Class로 정의하는데, Repository 같은 경우에는 크게 객체 자체가 어떤 상태를 가지거나 공유하지 않고 함수들도 동일한 입력에 대해 항상 동일한 출력만을 보장하기 때문에 Struct로 주로 사용하는 것 같아요 ㅎㅎ

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

오,, 감사합니다 제드!

"'상태'의 유무" 부분이 눈에 띄네요,,
사실 제가 사용한 모든 Repository들이 상태가 없는 경우가 많아서, 이런 부분 개선해보면 좋을 것 같아요ㅎㅎ 감사합니다!

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.

GoalDetail 수정 기능 적용, GoalDetail과 GoalAdd 뷰컨트롤러를 재사용가능하도록 수정

2 participants