[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 코드 분리 #340

sxunea · 2024-03-14T14:36:24Z

📌 개요

[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 순수하게 개선 #339
멀티 모듈로 이전하기 전, 데이터 레이어가 다른 레이어를 참조하지 않도록 코드를 분리합니다

✨ 작업 내용

이슈에 AS-IS, TO-BE 정리해두었으니 참고해주시면 됩니다 !
interceptor, authenticator 로 분리되어있던 기능 interceptor에 합치고, preference에 저장된 AccessToken의 값에 따라 화면 이동을 처리해 presentation 레이어의 로직을 분리했습니다

✨ PR 포인트

-> preference가 다른 곳에서도 상황에 따라 다양하게 저장되고 있더라고요 ! 헷갈릴까봐 참고로 정리해 두었습니다 ~ !

- 액세스 토큰 만료 됐을 때의 처리 - 리프레시 토큰 만료 됐을 때의 처리

unam98

수고하셨습니당

unam98 · 2024-03-17T03:30:23Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+    // 재발급 성공 : 저장
+    // 재발급 실패 : 재 로그인 토스트 메시지 띄우고 preference 빈 값 넣고 로그인 화면 이동
+    override fun intercept(chain: Interceptor.Chain): Response {
+        runBlocking { Timber.e("AccessToken : ${getAccessToken()}, RefreshToken : ${getRefreshToken()}") }


로그 찍는 이 부분을 runBlocking으로 감싸신 이유가 있나요?

unam98 · 2024-03-17T03:34:34Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+        chain: Interceptor.Chain
+    ): Response {
+        Timber.e("New Refresh Token Failure: ${refreshTokenResponse.code}")
+        saveToken("", "")


상황이 조금 더 직관적으로 파악될 수 있게 named argument 써보시는 건 어떨까요?

unam98 · 2024-03-17T03:35:26Z

app/src/main/java/com/runnect/runnect/di/RetrofitModule.kt

-        tokenAuthenticator: TokenAuthenticator
-    ): OkHttpClient = OkHttpClient.Builder().addInterceptor(logger).addInterceptor(appInterceptor)
-        .authenticator(tokenAuthenticator).build()


인터셉터 하나로 합치신 거 좋네요 굿

unam98 · 2024-03-17T03:38:37Z

app/src/main/java/com/runnect/runnect/presentation/login/LoginActivity.kt

+            // 빈 문자열 : 토큰 만료되어 재로그인
+            // none : 탈퇴, 로그아웃
+            // visitor : 방문자모드
+            // 나머지 : 바로 자동로그인 되므로 메인으로 이동


친절한 주석 좋아요~! 그런데 나중에 헷갈리거나 처음 보는 사람은 팔로우업이 어려울 수도 있을 것 같은데 저 값들이 다 String이면 Enum 클래스를 활용해보는 건 어떨까요? 그러면 개인적으로 주석 없이 상황을 좀 더 직관적이면서 가독성 좋게 표현할 수 있다고 생각합니다.

unam98 · 2024-03-17T03:41:19Z

app/src/main/java/com/runnect/runnect/presentation/login/LoginActivity.kt

+            if(accessToken.isBlank()){
+                Toast.makeText(
+                    this,
+                    getString(R.string.alert_need_to_re_sign),
+                    Toast.LENGTH_LONG
+                ).show()
+            }
+            else if (accessToken != "none" && accessToken != "visitor" ) {
+                Timber.d("자동로그인 완료")
+                moveToMain()
+                Toast.makeText(this@LoginActivity, MESSAGE_LOGIN_SUCCESS, Toast.LENGTH_SHORT).show()
+            }


가독성을 위해 이 부분 when문으로 처리하고 toast는 확장함수를 하나 만들어 써보는 건 어떨까요?

unam98 · 2024-03-17T03:51:03Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+    private fun getAccessToken(): String {
+        return PreferenceManager.getString(
+            ApplicationClass.appContext,
+            TOKEN_KEY_ACCESS
+        ) ?: ""
+    }
+
+    private fun getRefreshToken(): String {
+        return PreferenceManager.getString(
+            ApplicationClass.appContext,
+            TOKEN_KEY_REFRESH
+        ) ?: ""
+    }
+
+    private fun saveToken(accessToken: String, refreshToken: String) {
+        PreferenceManager.setString(ApplicationClass.appContext, TOKEN_KEY_ACCESS, accessToken)
+        PreferenceManager.setString(ApplicationClass.appContext, TOKEN_KEY_REFRESH, refreshToken)
+    }


이 부분 AuthUtils라는 SharedPreference 값 set/get 하는 목적의 유틸 클래스 하나 만들어서 거기로 옮기는 건 어떨까요? Login 쪽에서 getAccessToken() 함수 호출하는 부분이 있던데 유틸로 만들면 각각이 쓰여지는 부분을 좀 더 간결하게 만들 수 있을 것 같아서요

unam98 · 2024-03-17T04:08:19Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+        }
+    }
+
+    private fun getRefreshToken(originalRequest: Request, chain: Interceptor.Chain): Response {


accessToken을 재발급 받는 api인데 개인적으로는 이름이 getRefreshToken으로 돼있어서 조금 헷갈리는 것 같아 혹시 이름을 api endpoint와 통일해서 getNewToken으로 해보는 건 어떨까요?

ㅎㅎㅎ 공감합니다 전 코드가 refresh길래 일단 남겨둔거였어요 : ) 수정했습니다 !

unam98 · 2024-03-17T04:26:05Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+        when (response.code) {
+            CODE_TOKEN_EXPIRED -> {
+                try {
+                    Timber.e("Access Token Expired: getNewAccessToken")
+                    response.close()
+                    return handleTokenExpired(chain, originalRequest, headerRequest)
+                } catch (t: Throwable) {
+                    Timber.e("Exception: ${t.message}")
+                    saveToken("", "")
+                }
+            }
+        }
+        return response
+    }
+
+    private fun Request.newAuthTokenBuilder() =
+        runBlocking(Dispatchers.IO) {
+            val accessToken = getAccessToken()
+            val refreshToken = getRefreshToken()
+            newBuilder().apply {
+                addHeader(ACCESS_TOKEN, accessToken)
+                addHeader(REFRESH_TOKEN, refreshToken)
+            }
+        }


이 부분 더 쪼개서 구조를 아래처럼 바꿔보는 것은 어떨까요? 개인적으로 가독성을 더 높일 수 있다고 생각합니다.
*참고용으로 구조만 러프하게 잡은 거라 누락된 코드가 있습니다.(=기존의 logic이 그대로 유지되지 않았습니다.) 보충이 필요합니다.
ex) response.close, saveToken, exception catch 등

override fun intercept(chain: Interceptor.Chain): Response { val originalRequest = chain.request() val modifiedRequest = originalRequest.newAuthTokenBuilder().build() val response = chain.proceed(modifiedRequest) return if (response.code == CODE_TOKEN_EXPIRED) { handleTokenExpired(chain, originalRequest, modifiedRequest) } else { response } } private fun Request.newAuthTokenBuilder(): Request.Builder { return this.newBuilder().apply { val accessToken = getAccessToken() val refreshToken = getRefreshToken() addHeader(ACCESS_TOKEN, accessToken) addHeader(REFRESH_TOKEN, refreshToken) } }

넵 처음에 구현할 때 응답코드에 따른 다른 처리가 있을까봐 미리 when문으로 짜둔 건데 일단 지금은 불필요한것 같네요 !! 감사합니당 다만 말씀하신 의도가 return if로 response를 직접 리턴해주자는 게 맞나요 ? 쪼갠다는 맥락에 해당하는지는 잘모르겠어서 (ead0369) 에 수정해 푸쉬했으니 한번 확인부탁드려요 ~!

return if를 써보자는 것보다는 if문 안이 true일 때랑 false일 때 어떤 일이 일어나는지 간략하게 보여주고 구체적인 logic은 호출하는 함수 내부로 숨겨보자는 의미였습니다! (캡슐화)

unam98 · 2024-03-17T04:26:33Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+    }
+
+    private fun Request.newAuthTokenBuilder() =
+        runBlocking(Dispatchers.IO) {


이 부분 runBlocking 쓰신 이유가 있나요?

저 때는 해당 작업이 완료될때까지 스레드를 막으려고 runBlocking을 사용하긴 했습니다 ! 다만 runBlocking이 아예 차단이라는 점에서 스레드를 점유하니까 좋진 않을거라고 작성할때도 생각하긴했습니다 .. .ㅎ ㅎ 처음에는 suspend로 작성했었는데 그러면 newAuthTokenBuilder를 사용하는 okhttp3 intercept까지 suspend func이 되어야 하는데, 이를 지원하지 않더라구요 😭 그래서 차선책으로 사용했습니다!!

지금 생각해보니 저 동작이 꼭 비동기여야할 필요는 없다고 생각이 드는데 어떻게 생각하시나요 ? 아니면 혹시 runBlocking을 쓰지 않고 다르게 처리할 방법이 있는지 궁금합니다

혹시 runBlocking을 작성하지 않았을 때 제대로 동작하지 않았나요??
interceptor가 비동기적으로 동작하더라도 같은 메소드 내에서는 실행 순서가 보장될테니 별도 처리가 필요 없을 것 같다는 생각이 들었습니다~!

처음에 구현할땐 저도 필요없다고 생각했어서 작성하지 않았는데 동작이 안됐어서 runBlocking을 사용했는데, 지금 제거하고 보니 또 잘 가져오네요 🥹 다른 부분이 잘못됐었는데 불필요하게 같이 고쳐준 것 같아요 저도 runBlocking이 여전히 불필요하다고 생각합니다 ㅎㅎㅎ 없애서 수정할게요 !

dongx0915

고생하셨습니다~

dongx0915 · 2024-03-24T16:18:00Z

app/src/main/java/com/runnect/runnect/data/service/AuthInterceptor.kt

+    }
+
+    private fun Request.newAuthTokenBuilder() =
+        runBlocking(Dispatchers.IO) {


혹시 runBlocking을 작성하지 않았을 때 제대로 동작하지 않았나요??
interceptor가 비동기적으로 동작하더라도 같은 메소드 내에서는 실행 순서가 보장될테니 별도 처리가 필요 없을 것 같다는 생각이 들었습니다~!

.../com/runnect/runnect/presentation/mypage/setting/accountinfo/MySettingAccountInfoFragment.kt

app/src/main/java/com/runnect/runnect/presentation/MainActivity.kt

# Conflicts: # app/src/main/java/com/runnect/runnect/di/RetrofitModule.kt

sxunea added 7 commits March 14, 2024 22:37

[FEAT] #339 Interceptor에서 액세스, 리프레시 토큰 관리 작업 구현

a2d3c2b

- 액세스 토큰 만료 됐을 때의 처리 - 리프레시 토큰 만료 됐을 때의 처리

[FEAT] #339 액세스 토큰 재발급 성공시, preference에 해당 값 저장

b609e6c

[FEAT] #339 리프레시 토큰 또한 만료 / 통신 실패 시 빈 값 preference에 저장

8b882b7

[CHORE] #339 주석 추가 및 코드 정리

123e7e7

[DEL] #339 Authenticator 제거

60adff3

[MOD] #339 레트로핏 모듈에 Interceptor 반영

3e8d971

[FEAT] #339 accessToken 빈 문자열이면 토스트메시지와 함께 재로그인 요청

1aebc18

sxunea added REFACTOR 🧹 코드 리팩토링 혜선 🐱 혜선 담당 labels Mar 14, 2024

sxunea requested review from dongx0915, leeeha and unam98 March 14, 2024 14:36

sxunea self-assigned this Mar 14, 2024

[REFACTOR] #339 non null 단언 연산자 제거

2ac7be5

unam98 approved these changes Mar 17, 2024

View reviewed changes

sxunea added 6 commits March 24, 2024 15:27

[REFACTOR] #339 named argument 적용

0c210d0

[REFACTOR] #339 interceptor return if로 response처리

ead0369

[REFACTOR] #339 리뷰 간단사항 반영

60bbd81

[ADD] #339 LoginStatus Enum Class 만들어 적용해 토큰 값 따른 로그인 상태 관리

a5eab3d

[REFACTOR] #339 토큰preferenceManager AuthUtil로 빼서 관리

b3267c8

[MOD] #339 applicationContext 주입

625d913

sxunea requested a review from unam98 March 24, 2024 10:44

dongx0915 reviewed Mar 25, 2024

View reviewed changes

sxunea added 2 commits March 27, 2024 16:34

[MOD] #339 runBlocking제거

e9c245b

[REFACTOR] #339 applicationContext -> context로 변경하고, null 안전하게 체크

0af3516

dongx0915 approved these changes Apr 13, 2024

View reviewed changes

leeeha approved these changes Apr 23, 2024

View reviewed changes

sxunea added 2 commits April 24, 2024 16:07

Merge remote-tracking branch 'origin/develop' into feature/refactor-data

a390166

# Conflicts: # app/src/main/java/com/runnect/runnect/di/RetrofitModule.kt

[MERGE] #339 충돌 해결

7315ecc

sxunea merged commit 5661733 into develop Apr 24, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 코드 분리 #340

[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 코드 분리 #340

sxunea commented Mar 14, 2024 •

edited

Loading

unam98 left a comment

unam98 Mar 17, 2024

unam98 Mar 17, 2024

unam98 Mar 17, 2024

unam98 Mar 17, 2024

unam98 Mar 17, 2024

unam98 Mar 17, 2024

unam98 Mar 17, 2024

sxunea Mar 24, 2024

unam98 Mar 17, 2024

sxunea Mar 24, 2024 •

edited

Loading

unam98 Mar 29, 2024

unam98 Mar 17, 2024

sxunea Mar 24, 2024

dongx0915 Mar 24, 2024

sxunea Mar 27, 2024

dongx0915 left a comment

dongx0915 Mar 24, 2024

[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 코드 분리 #340

[REFACTOR] 데이터 레이어 다른 계층 참조하지 않도록 코드 분리 #340

Conversation

sxunea commented Mar 14, 2024 • edited Loading

📌 개요

✨ 작업 내용

✨ PR 포인트

unam98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sxunea Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongx0915 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sxunea commented Mar 14, 2024 •

edited

Loading

sxunea Mar 24, 2024 •

edited

Loading