[Refactor] Oauth 쪽 불필요한 Flow 제거 #383
Conversation
Summary of ChangesHello @HI-JIN2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 OAuth 관련 API 호출 비즈니스 로직에서 불필요한 Flow 사용을 제거하여 코드의 복잡성을 줄이고 가독성을 높이는 리팩토링을 수행합니다. 데이터 계층에서 도메인 계층으로의 변환을 명확히 하고, 리포지토리 및 유스케이스의 반환 타입을 단순화함으로써, 특히 ViewModel에서 비동기 작업 및 에러 처리를 더욱 효율적으로 관리할 수 있도록 개선했습니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 리팩터링을 통해 Oauth 관련 로직에서 Flow를 제거하고 suspend 함수를 직접 사용하도록 변경하여 코드를 간소화한 점이 좋습니다. 특히 ViewModel에서 runCatching을 사용하여 비동기 작업을 처리하는 방식은 좋은 개선입니다. 다만, 리팩터링 과정에서 발생한 몇 가지 잠재적인 버그를 발견했습니다. Repository 구현체에서 API 호출 실패 시 빈 Token 객체를 반환하는 부분은 예외를 던지는 방식으로 수정해야 합니다. 또한, TokenAuthenticator에서 토큰 유효성을 검사하는 로직이 현재는 올바르게 동작하지 않아 수정이 필요합니다. 자세한 내용은 각 파일에 남긴 코멘트를 참고해주세요.
app/src/main/java/com/eatssu/android/di/network/TokenAuthenticator.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/data/repository/OauthRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/eatssu/android/data/repository/OauthRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
flow를 안씀으로써 훨씬 깔끔하네요!
다만 제미니가 말한 것처럼 OauthRepositoryImpl에서 결과값이 null일 때 Token("", "")를 반환하니 TokenAuthenticator에서도 null인지가 아니라 비어있는지를 확인해야할 것 같아요.
아니면 대안으로 TokenAuthenticator.authenticate가 try-catch로 오류에 대응하고 있으니 OAuth 관련 Use Case가 throw할 수 있다는 것을 확실히 주석으로 작성하거나 애초에 Token?를 반환하면 좋다고 생각합니다!
kangyuri1114
left a comment
kangyuri1114
left a comment
There was a problem hiding this comment.
이전 pr, 해당 pr 모두 보니 불필요 flow 제거하고 runcatching 도입으로 리팩중이시군요
앞으로는 이런 식으로 전체적으로 같이 리팩하면 좋겠네요 👍
runCatching {
withContext(Dispatchers.IO) {
app/src/main/java/com/eatssu/android/di/network/TokenAuthenticator.kt
Outdated
Show resolved
Hide resolved
|
@PeraSite @kangyuri1114 코드리뷰 반영했습니다! 익셉션 던지도록 하였어요! 한가지 고민 중인게 있는데, runCatching이랑 try-catch 중에 선호하는 방식이나 견해를 묻고 싶습니다! |
|
try catch 자체가 컨트롤 플로우가 끊어지는 구조라서 error as value로 흐름이 깔끔한 runCatching가 더 좋은 것 같아요! |
|
저도 runCatching 선호합니다~! |
Summary
OauthServie에 해당되는 API 호출 비즈니스로직에서 불필요한 flow를 제거합니다.