-
Notifications
You must be signed in to change notification settings - Fork 3
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: 알송파트 유즈케이스 적용 #666
feat: 알송파트 유즈케이스 적용 #666
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.
와우 알송 많은 코드를 수정해주셨네요...!
너무 많은 양이어서 일단 코멘트 남기고 나중에 더 남길게요!!
|
||
@Qualifier | ||
@Retention(AnnotationRetention.BINARY) | ||
annotation class CheckAlreadyLoggedInUseCaseQualifier |
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 구현체가 1개 인것으로 하는데 굳이 Qualifier를 설정하는 이유가 궁금해요!!
제가 모르는 다른 이유가 있나요? 🧐
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.
일단은 구현체가 추가될 것에 대비해서 Qualifier를 설정 해두었습니다.
usecase를 추상화 하지 않아도 된다라는 피드백이라면, 목요일에 서기와도 얘기를 했던 부분인데요,
https://mashup-android.vercel.app/mashup-11th/heejin/useCase/useCase/
위 링크에서 usecase를 interface로 추상화 했길래, 저도 추상화 했습니다.
서기도 usecase를 추상화 할 필요까지는 없다는 의견을 주었는데요, 저도 대체적으로 동의하지만 저는 usecase를 처음 적용해 보는 것이기도 해서 위 블로그 내용을 최대한 따라하려고 노력했고, 또 확장성의 관점에서 usecase를 추상화 함으로써 비즈니스 로직이 바뀔 경우 구현체만 교체하는 방식으로 수정이 용이해지는 장점이 있다고 생각해서 이렇게 구현했습니다. 서기와 채채는 usecase를 추상화 하지 않고 구현해도 된다고 생각합니다!
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에 대한 구현체가 여러개가 된다는 가정이 필요없다는 의견이어서 이렇게 코멘트를 남겼습니다! 불필요한 복잡도가 늘어나는 것을 경계해야한다고 생각하기 때문에!! 하지만 알송의 코멘트를 통해서 알송의 생각을 알게되었숩니다 감사해요~
when (result.error) { | ||
DataError.Network.UNAUTHORIZED -> { | ||
when (authRepository.saveRefresh()) { | ||
is Result.Success -> invoke(multipartBody) |
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 적용 전과 똑같이 동작한다고 생각되는데, 그렇지 않은 걸까요?
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.
무한 재귀를 돌 가능성이 생겨서 최대한 지양하고 싶었는데, 뾰족한 수가 따로 없네요...! 백엔드와 상의해서 계속해서 saveRefresh를 요청하는 유저에 대해 어떻게 처리해야할지 논의해봐야겠서오,,,!
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의 추상화에 대해서 5년전 아티클이긴한데 여기 댓글에서도 이걸 주제로 의견을 주고받았더라고요. 한번 참고해보면 좋을 것 같아서 링크 남겨요!
https://proandroiddev.com/why-you-need-use-cases-interactors-142e8a6fe576
📌 관련 이슈
close #663
✨ 작업 내용
알송파트에 유즈케이스 적용했습니다 다음 뷰모델에 적용되었습니다
📚 기타
다른 분들도 유즈케이스 적용하시다보면 Hilt로 유즈케이스를 뷰모델에 주입해야 할텐데, 제가 만들어 둔 Qualifier, DependencyModule 파일에다가 추가로 작성하시면 됩니다. (아직 이슈가 안올라온 것으로 보아 다른 사람들 아직 시작 안한것으로 판단함)