-
Notifications
You must be signed in to change notification settings - Fork 0
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
task #273 채널 생성 및 삭제 시점 변경 #275
Conversation
UserDefaults.standard.set(channelEntity.id, forKey: "CHANNEL_ID") | ||
let savedID = UserDefaults.standard.string(forKey: "CHANNEL_ID") | ||
|
||
self?.output.isSaved.send(savedName == name && savedID != 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.
self?.output.isSaved.send(savedName == name && savedID != nil) | |
self?.output.isSaved.send(savedName == name && savedID == channelEntity.id) |
nil 값 확인이 아닌 channelEntity.id
와 비교하는 것도 좋을 것 같아요.
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
를 멋지게 갈아끼워서 넣으셨군요~!~! 👍 유효성 검사 부분에서도 많이 생각하신게 보여서 넘 인상적으로 봤습니당 👍
createChannelUsecase: createChannelUsecase, | ||
deleteChannelUsecase: deleteChannelUsecase, |
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 2개가 덜어졌네요!! 👀🥹👍
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.
👍🏻
private var channelDescription: String = "" | ||
private var channel: ChannelEntity? | ||
private var channelID = UserDefaults.standard.string(forKey: "CHANNEL_ID") |
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.
이렇게 하면 init 될때만 가져오기때문에 당장은 문제가 없지만 추후 채널 아이디가 변경될 경우 (새로운 채널을 생성했을때?) 고려해야할 부분 같습니다.
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를 실행하는 메소드를 따로 빼서 구현되어 있거나, input으로 스트림을 처리할 때 진행하던데 위와 같이하는게 좋을지 따로 메소드로 빼는게 좋을지 고민했습니다.
결과적으로 output으로 보낼 때 닉네임이 제대로 저장되었는지와 채널 id가 저장되었는지 2개를 확인해야했기에 하나의 메소드 안에 같이 있는게 좋을 것 같다고 판단했습니다.
보시기에 어떠신가요?
✅ PR 체크리스트
XCConfig
,노션
,README
)"API 개발 완료됐어요"
,"XCConfig 값 추가되었어요"
)🎸 기타