-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feat] #12 - 회원가입 뷰 구현 #23
The head ref may contain hidden characters: "feat/#12-\uD68C\uC6D0\uAC00\uC785\uBDF0-\uAD6C\uD604"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깔끔하게 잘 짜셨네요~~ 코드 하나도 안더러운데 ㅋㅋㅋ 기만
output.nicknameAlert.sink { event in | ||
print("event: \(event)") | ||
} receiveValue: { [weak self] alertText in | ||
guard let self = self else { return } | ||
self.nickNameTextFieldView.changeAlertLabelText(alertText) | ||
if !alertText.isEmpty { | ||
self.nickNameTextFieldView.setTextFieldViewState(.alert) | ||
} | ||
}.store(in: cancelBag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기서 sink를 통해 output을 구독하고, 이러한 값들을 textFieldView에 반영하고 있는데
assign을 이용해 binding을 조금 더 깔끔하게 할 수 있는 방법도 있습니다(자료).
example(of: "assign") {
// 1. didSet을 통해 value 값이 바뀌면 새 값을 print합니다.
class SomeObject {
var value: String = "" {
didSet {
print(value)
}
}
}
// 2. 위에서 만든 class의 instance를 선언합니다.
let object = SomeObject()
// 3. String 배열로 이뤄진 publisher를 생성합니다.
let publisher = ["Hello", "world!"].publisher
// 4. publisher를 구독하면서 새롭게 받은 값을 object의 value에 할당합니다.
_ = publisher
.assign(to: \.value, on: object)
}
자료를 참고하시면 위와 같은 코드가 있는데, assign을 이용해서 아래와 같이 할 수도 있을 것 같아요!
extension CustomTextFieldView {
var alertText: String {
get { return alertlabel.text ?? "" }
set { bindAlertText(newValue) }
}
private func bindAlertText(_ alertText: String) {
self.changeAlertLabelText(alertText)
if !alertText.isEmpty {
self.setTextFieldViewState(.alert)
}
}
}
output.nicknameAlert
.assign(to: \.alertText, on: textFieldView)
.store(in: self.cancellBag)
공통 컴포넌트이기에 binding에 대한 코드를 줄이려면 위와 같은 방법이 유용하겠네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가적으로 enum을 이용해서 input을 구조화 시킬 수 있을 것 같아요~~!
currentText는 대충 만든 거니 InputCase 안쪽에 bind 할 수 있는 케이스들을 만들어주면 될 것 같습니다!
extension CustomTextFieldView {
func bindableInput<T>(_ input: InputCase) -> ReferenceWritableKeyPath<CustomTextFieldView, T> {
return input.keyPath as! ReferenceWritableKeyPath<CustomTextFieldView, T>
}
enum InputCase {
case alert
case currentText
var keyPath: AnyKeyPath {
switch self {
case .alert: return \CustomTextFieldView.alertText
case .currentText: return \CustomTextFieldView.textChanged
}
}
}
}
let nicknameAlert = ["안녕하세요", "경고입니다"].publisher
micknameAlert
.assign(to: tf2.bindableInput(.alert), on: tf2)
.store(in: self.cancelBag)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assign(to:) 의 경우 Publisher의 Failure type이 Never여야 사용이 가능해서
var nicknameAlert = PassthroughSubject<String, Error>()
현재는 이처럼 Failure type이 Error이기 때문에 사용이 불가능합니다 ㅠ
이걸 Never로 바꾸면 사용이 가능하지만 옳은 방식인지는 잘 모르겠습니다..
Never로 바꾸고 assign을 사용해도 될지 궁금합니다...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우와 keyPath는 아직 익숙하지 않아서 코드 이해하는 데 한참 걸렸는데
확실히 훨씬 깔끔하게 짤 수 있겠네요..!!!! 감사합니다 ㅠㅠ
view에서도 ViewModel 처럼 Input을 구조화한다는 아이디어 너무 좋습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsj8706 nicknameAlert는 viewModel에서 나오는 output이고, 뷰모델에서 output이 방출된다면 그 상태로 error 케이스가 없는 것이 타당하다고 생각합니다. 현재 로직에서도 Error 타입을 통해서 Error에 대한 분기처리를 해주고 있지 않기 때문에, Error에 대한 핸들링이 필요하다면 UseCase의 output에서 발생할 수 있는 Error를 ViewModel에서 처리하고 이를 Error가 없는 Never 타입으로 만들어 ViewModel의 Output에 부여할 수 있겠다는 생각이 들어요!
현재 아키텍쳐에서는 비즈니스 로직은 최대한 ViewModel에게 넘기고 있기 때문에 이러한 처리를 뷰모델에서 모두 하는 것이 좋은 것 같습니다. 저번 SOPT-iOS 프로젝트에서 PostDetailViewModel을 보시면 Input은 Driver, Output은 @published를 통해서 처리하고 있는 것을 보실 수 있을 거에요! 이 방식은 Github 다른 레포에서 찾은 부분이며, 모두 Error에 대한 케이스 처리가 필요 없기 때문에 사용한 방식이라고 생각합니다.
말이 길어졌지만 ViewModel에서 Error에 대한 처리를 해준다면 Output에는 Driver 또는 @published를 사용해도 된다고 생각해요! 그렇게 하면 assign을 사용할 수 있습니다! 어쩌다보니 KeyPath는 자연스럽게 써버렸는데 저도 어제 공부하면서 쓴거라 이해가 부족하지만, KVC 패턴에 대해 검색해 보면 많은 내용이 나옵니다~~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@L-j-h-c 어우 꼼꼼한 피드백 너무너무 감사합니다!!! 👍
확실히 ViewModel의 Output에서는 Error 없이 내보내는 것이 맞아보이네요!
이 부분 다듬어서 다시 push 하겠습니다!!!
Combine 공부하다 보면 KVO, KVC 이야기가 많이 나오던데 저도 한번 공부해 보겠습니다!!
useCase.isNicknameValid.combineLatest( | ||
useCase.isEmailFormValid, | ||
useCase.isPasswordFormValid, | ||
useCase.isAccordPassword) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 방식도 가능하지만 CurrentValueSubject를 사용해서 값들을 판단해주는 방법도 있습니다! 그러면 새로운 publisher를 만들 필요가 없어요!
그리고 현재 로직상 Usecase에 있는 프로퍼티들을 많이 참조하고 있는데, 결합도를 줄이기 위해 UseCase에서 validation을 하고, 뷰모델에서는 그 값을 받아와서 isValidForm에 전달만 하는 식으로 하면 좋을 것 같습니다!
.map { (isNicknameValid, isEmailValid, isPasswordValid, isAccordPassword) in
(isNicknameValid && isEmailValid && isPasswordValid && isAccordPassword)
}
위의 부분을 유즈케이스에서 하는게 맞는 것 같다는 생각이에요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반영했습니다!
.combineLatest(input.passwordCheckTextChanged.compactMap({ $0 })) | ||
.sink { (firstPassword, secondPassword) in | ||
self.useCase.checkAccordPassword(firstPassword: firstPassword, secondPassword: secondPassword) | ||
}.store(in: self.cancelBag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 부분에서는 currentValueSubject를 쓰면 combinelatest를 쓰지 않아도 됩니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨슴다 나도 빨리 해야쥐..
public class SignUpRepository { | ||
|
||
private let networkService: AuthService | ||
private let cancelBag = Set<AnyCancellable>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CancelBag 저번에 만들어놓은거 쓰면 될거같슴다
} | ||
|
||
func checkEmailForm(email: String) -> Bool { | ||
let emailRegEx = "[A-Z0-9a-z._%+-]+@[A-Za-z0-9.-]+\\.[A-Za-z]{2,64}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oㅑ무zl네yo
private func setKeyboardNotification() { | ||
NotificationCenter.default.addObserver( | ||
self, | ||
selector: #selector(keyboardWillShow), | ||
name: UIResponder.keyboardWillShowNotification, | ||
object: nil) | ||
|
||
NotificationCenter.default.addObserver( | ||
self, | ||
selector: #selector(keyboardWillHide), | ||
name: UIResponder.keyboardWillHideNotification, | ||
object: nil) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
키보드 노티 쓰는 코드 다른데에서도 쓰일거같은데 따로 빼는건 어떠하신가욥??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 🙇♀️
useCase.isPasswordFormValid.combineLatest(useCase.isAccordPassword).sink { event in | ||
print("SignUpViewModel - completion: \(event)") | ||
} receiveValue: { (isFormValid, isAccordValid) in | ||
if !isFormValid && !isAccordValid { | ||
output.passwordAlert.send(I18N.SignUp.invalidPasswordForm) | ||
} else if !isFormValid && isAccordValid { | ||
output.passwordAlert.send(I18N.SignUp.invalidPasswordForm) | ||
} else if isFormValid && !isAccordValid { | ||
output.passwordAlert.send(I18N.SignUp.passwordNotAccord) | ||
} else { | ||
output.passwordAlert.send("") | ||
} | ||
}.store(in: cancelBag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsj8706
수정하신 부분 잘 봤습니다!! 참고로 useCase의 참조를 줄인다는건 아래 코드와 같은 의미였어요 ㅎㅎ 이런 방식도 고려해보셔도 될 것 같아요!
public class DefaultSignUpUseCase {
private func makeCurrentPasswordAlert() {
self.isPasswordFormValid.combineLatest(self.isAccordPassword).sink { event in
print("SignUpUseCase - completion: \(event)")
} receiveValue: { (isFormValid, isAccordValid) in
if !isFormValid && !isAccordValid {
currentPasswordAlert.send(I18N.SignUp.invalidPasswordForm)
} else if !isFormValid && isAccordValid {
currentPasswordAlert.send(I18N.SignUp.invalidPasswordForm)
} else if isFormValid && !isAccordValid {
currentPasswordAlert.send(I18N.SignUp.passwordNotAccord)
} else {
currentPasswordAlert.send("")
}
}.store(in: cancelBag)
}
}
public class SignUpViewModel {
private func bindOutput(output: Output, cancelBag: CancelBag) {
useCase.currentPasswordAlert.sink { event in
print("SignUpViewModel - completion: \(event)")
} receiveValue: { alert in
output.passwordAlert.send(alert)
}.store(in: cancelBag)
}
}
🌴 PR 요약
🌱 작업한 브랜치
🌱 PR Point
combineLatest
를 사용했는데 혹시 더 좋은 방법이 있다면 알려주세요!!!📌 참고 사항
📸 스크린샷
📮 관련 이슈