[REFACTOR] 마이페이지 리팩토링 #357

dongx0915 · 2024-06-16T15:25:29Z

📌 개요

Closed : [REFACTOR] 마이페이지 리팩토링 #356

✨ 작업 내용

Compose로 변환하기 전 마이페이지 코드를 리팩토링 하였습니다.
해당 코드를 베이스로 Compose 변환을 해도 되고, 아니면 직접 마이페이지 리팩토링 후 변환 진행해도 좋을 것 같습니다~

✨ PR 포인트

📸 스크린샷/동영상

unam98

굿 work~!

unam98 · 2024-06-17T01:25:32Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageViewModel.kt

-                    }
-
-                    _userInfoState.value = UiState.Success
+                    val userDto = UserDto.toDto(user)


UserRepositoryImpl에서 이미 User 타입으로 맵핑을 시켜주고 있는데 여기서 한 번 더 맵핑을 시켜주는 이유가 있나요?

@unam98
Repository는 Data 레이어이기 때문에 Presentation 레이어의 Dto를 참조할 수 없습니다
그래서 Presentation 레이어에서 추가적인 매핑을 처리 해주었습니다

unam98 · 2024-06-17T01:30:56Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+        addObserver()
+        setResultEditNameLauncher()
+
+        viewModel.getUserInfo()


viewModel.getUserInfo() vm = viewModel lifecycleOwner = this@MyPageFragment.viewLifecycleOwner

개인적인 생각인데 이 부분 각각 별도의 함수로 분리해서 deactivateVisitorMode() 함수 내부의 추상화 수준을 다른 것들과 맞춰주는 건 어떨까요?

@unam98 함수 내부의 추상화 수준을 맞춘다는게 어떤 뜻인지 설명해줄 수 있을까요?

@unam98 추가로 어차피 Compose로 리팩토링할 화면이기 때문에 제거될 코드 (dataBinding 등)는 그냥 유지했습니다~

private fun deactivateVisitorMode() { setVisitorMode(false) addListener() addObserver() setResultEditNameLauncher() viewModel.getUserInfo() with(binding) { vm = viewModel lifecycleOwner = this@MyPageFragment.viewLifecycleOwner } }

예를 들어서 setVisitorMode()부터 setResultEditNameLauncher()까지는 내부 로직이 드러나지 않게 함수로 감싸져있지만 아래부터는 내부 로직이 그대로 드러나는 모양이라 아랫부분들도 위처럼 맞춰주자는 것이었습니다. 추상화 수준이라는 표현보다는 캡슐화 수준이라는 표현이 좀 더 와닿을 수도 있겠네요.

클린코드라는 책에서 봤던 내용인데 이 책이 항상 정답은 아니겠지만 함수 분리를 통한 장점도 누리면서 좀 더 깔끔한 것 같아 개인적인 의견 내봤습니당.

unam98 · 2024-06-17T01:31:49Z

app/src/main/java/com/runnect/runnect/presentation/mypage/dto/UserDto.kt

+import com.runnect.runnect.R
+import com.runnect.runnect.domain.entity.User
+
+data class UserDto(


User라는 entity가 이미 있는데 UserDto를 또 만드신 이유가 궁금합니당

@unam98 profileImgResId가 추가로 필요해서 Dto를 사용하는게 맞다고 생각하여 추가했습니다

leeeha

리팩토링 고생하셨습니다!

leeeha · 2024-06-19T11:39:42Z

app/src/main/java/com/runnect/runnect/presentation/base/BaseViewModel.kt

+        data class ShowSnackBar(val message: String) : EventState
+        data class NetworkError(val message: String) : EventState
+        data class UnknownError(val message: String) : EventState
+    }


매~~우 사소하고 주관적인 의견이지만! EventState라는 네이밍이 약간 고민되네요!

보통 SharedFlow는 여러 이벤트를 처리할 때, StateFlow는 수신자를 최신 상태로 갱신할 때 사용하는 것으로 알고 있습니다. 그래서 EventState보다는 Event 또는 EventBus 같은 네이밍은 어떨까라는 생각이 들었어요.

EventState라고 하면 이게 이벤트인지, 상태인지 아주 약간 헷갈릴 수도 있을 거 같아서요 :)

참고로 SharedFlow 내부 코드에서도 아래처럼 EventBus 라는 클래스가 있더라구요!

class EventBus { private val _events = MutableSharedFlow<Event>() // private mutable shared flow val events = _events.asSharedFlow() // publicly exposed as read-only shared flow suspend fun produceEvent(event: Event) { _events.emit(event) // suspends until all subscribers receive it } }

@leeeha 오호 좋은 것 같습니다
UI State와 1회성 이벤트를 구분하기 위해 SharedFlow를 추가했으니 네이밍도 확실히 구분되면 좋을 것 같네요

EventBus는 파이프라인이라는 인식이 좀 강한 것 같아서 Event 케이스를 나누는 목적과는 쪼금 차이가 있는 것 같긴하네요

한 번 고민해보고 수정해볼게요 좋은 네이밍 생각나면 말씀해주세요~

leeeha · 2024-06-19T11:47:04Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+
    private lateinit var resultEditNameLauncher: ActivityResultLauncher<Intent>
-    var isVisitorMode: Boolean = MainActivity.isVisitorMode
+    private var isVisitorMode: Boolean = MainActivity.isVisitorMode


방문자 모드 체크할 때 이런 식으로 companion object로 처리하는 게 최선인지 고민되더라구요,,
shared preference 사용하는 것도 괜찮을 거 같고,, 이런 데이터는 어떻게 처리하는 게 현명할까요??

@leeeha
저도 이 부분에 대해 깊게 고민해보지는 않았지만 Shared Preference 등을 이용하는게 가장 깔끔할 것 같습니다
대신 Presentation 레이어에서 Preference를 참조하는 것이 아닌 Data 레이어로 옮기는게 좋아보이네요!

방문자 모드인지를 판별하는 로직도 ViewModel로 들어가야하지 않을까 싶습니다.

shared preference를 사용하는 것이 기존의 companion object를 사용하는 방법과 비교했을 때 어떤 장점이 있을까요?

기존 코드에서 고민이 되었던 건 '굳이 저 변수를 메인 액티비티의 전역 변수로 만들어야 하는가?' 였습니다.
지금 상태로는 isVisitorMode 라는 변수가 메인 액티비티의 생명주기를 따르게 될텐데
프로그램 전역으로 사용되는 거라면 차라리 로컬 저장소인 shared preferences를 사용하는 게 낫지 않을까? 라는 생각이 들었어요!

현재 코드를 살펴보니 액세스 토큰 값을 기준으로 방문자 모드 여부를 체크하던데, 그 결과값을 메인 액티비티의 전역 변수가 아닌 shared preferences에 저장해두고 사용해도 편할 거 같다는 생각이 들었습니다~!
isVisitorMode 변수 자체는 뷰의 라이프사이클에 종속될 필요가 없는 데이터인 거 같아서요 :)

leeeha · 2024-06-19T11:54:02Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+                activity?.let {
+                    Intent(it, LoginActivity::class.java).let(::startActivity)
+                    it.finish()
+                }


reflection을 평소에 자주 사용하시는 거 같은데 특별한 이유가 있는지 궁금합니다!

@leeeha
그냥 코드가 간단해져서 자주 씁니다😄

궁금해서 GPT한테 물어봤어요 ㅎㅎ

장점

런타임 동적 처리: 리플렉션을 사용하면 프로그램 실행 중에 동적으로 클래스, 메서드, 프로퍼티 등을 검사하고 조작할 수 있습니다. 이를 통해 유연하고 동적인 프로그래밍이 가능합니다.

코드 생성 및 분석: 리플렉션을 활용하면 코드를 동적으로 생성하거나 분석할 수 있습니다. 이는 프레임워크나 라이브러리 개발 시 유용합니다.

디버깅 및 테스트 지원: 리플렉션을 통해 객체의 내부 구조를 검사할 수 있어 디버깅과 테스트에 도움이 됩니다.

단점

성능 저하: 리플렉션은 일반적인 메서드 호출보다 성능이 떨어집니다. 따라서 성능이 중요한 코드에서는 사용을 자제해야 합니다.

컴파일 타임 타입 검사 약화: 리플렉션을 사용하면 컴파일 타임에 타입 검사가 어려워지므로, 런타임 오류가 발생할 가능성이 높아집니다.

코드 가독성 저하: 리플렉션을 과도하게 사용하면 코드가 복잡해지고 가독성이 떨어질 수 있습니다.

보안 위험: 리플렉션을 통해 악의적인 코드 실행이 가능하므로 보안 측면에서 주의가 필요합니다.

leeeha · 2024-06-19T11:58:43Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

-                    viewModel.setNickName(name!!)
+                    val name = result.data?.getStringExtra(EXTRA_NICK_NAME) ?: ""
+                    viewModel.updateUser(
+                        userData.copy(nickName = name)


var을 사용하지 않을 수 있는 방법이어서 좋은 거 같습니다!
그런데 객체 하나가 담고 있는 데이터가 많다면 복사하는데 시간이 조금 걸릴 수도 있을 거 같다는 생각이 드네요!
트레이드 오프가 있는 거 같은데, 현재는 객체 복사 시간이 그렇게 오래 걸릴 거 같지 않아서 지금 방법이 괜찮아보입니다 :)

@leeeha
좋은 의견인 것 같습니다.
기본적으론 불변성을 유지하는 방안으로 작업하고 데이터가 크거나 많은 경우에는 불변성을 포기하는 방법도 고려해봐야할 것 같네요!

leeeha · 2024-06-19T12:00:34Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+                    when (it) {
+                        is BaseViewModel.EventState.ShowSnackBar -> {
+                            context?.showSnackbar(binding.root, it.message)
+                        }


스낵바가 반복적으로 뜨는 문제.. 위니에서도 발생했었는데 덕분에 해결 방법 알 수 있었네요! 저도 얼른 적용해봐야겠어요!

leeeha · 2024-06-19T12:01:51Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+        activity?.let {
+            Intent(it, activityClass).let(::startActivity)
+            it.applyScreenEnterAnimation()
        }


ActivityExt.kt 파일에 확장함수로 만들어둬도 좋을 거 같습니다!

leeeha · 2024-06-19T12:05:38Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageViewModel.kt

-                    errorMessage.value = it.toLog()
-                    _userInfoState.value = UiState.Failure
+                    _userState.value = UserState.Failure(it.toLog())
+                    sendEvent(EventState.ShowSnackBar(it.toLog()))


toLog() 라는 확장함수를 만들어두셨군요! 굿굿

unam98 · 2024-06-23T23:45:34Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+        addObserver()
+        setResultEditNameLauncher()
+
+        viewModel.getUserInfo()


private fun deactivateVisitorMode() { setVisitorMode(false) addListener() addObserver() setResultEditNameLauncher() viewModel.getUserInfo() with(binding) { vm = viewModel lifecycleOwner = this@MyPageFragment.viewLifecycleOwner } }

예를 들어서 setVisitorMode()부터 setResultEditNameLauncher()까지는 내부 로직이 드러나지 않게 함수로 감싸져있지만 아래부터는 내부 로직이 그대로 드러나는 모양이라 아랫부분들도 위처럼 맞춰주자는 것이었습니다. 추상화 수준이라는 표현보다는 캡슐화 수준이라는 표현이 좀 더 와닿을 수도 있겠네요.

클린코드라는 책에서 봤던 내용인데 이 책이 항상 정답은 아니겠지만 함수 분리를 통한 장점도 누리면서 좀 더 깔끔한 것 같아 개인적인 의견 내봤습니당.

unam98 · 2024-06-23T23:46:41Z

app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt

+
    private lateinit var resultEditNameLauncher: ActivityResultLauncher<Intent>
-    var isVisitorMode: Boolean = MainActivity.isVisitorMode
+    private var isVisitorMode: Boolean = MainActivity.isVisitorMode


shared preference를 사용하는 것이 기존의 companion object를 사용하는 방법과 비교했을 때 어떤 장점이 있을까요?

dongx0915 added 6 commits June 16, 2024 23:50

[refactor] MyPage 코드 리팩토링 (Livedata -> StateFlow, SharedFlow)

f5f6129

[feat] : UserDto 추가

b915452

[mod] : 데이터바인딩 코드 수정

4ca09e8

[feat] : BaseViewModel에 EventState 추가 (SharedFlow)

6f37a31

[mod] : UserState Success에서 user 파라미터 제거

98bab4f

[mod] : UserState.Success -> UserUpdated로 변경

2163843

dongx0915 added REFACTOR 🧹 코드 리팩토링 동현 🐨 동현 담당 labels Jun 16, 2024

dongx0915 requested review from leeeha, sxunea and unam98 June 16, 2024 15:25

dongx0915 self-assigned this Jun 16, 2024

dongx0915 changed the base branch from feature/add-eventstate-to-baseviewmodel to develop June 16, 2024 15:25

unam98 reviewed Jun 17, 2024

View reviewed changes

unam98 force-pushed the develop branch from 82331c2 to cf0cb7d Compare June 17, 2024 13:26

leeeha approved these changes Jun 19, 2024

View reviewed changes

unam98 force-pushed the develop branch 2 times, most recently from fb1f98f to f8525c2 Compare June 19, 2024 15:17

unam98 approved these changes Jun 23, 2024

View reviewed changes

[REFACTOR] 마이페이지 리팩토링 #357

Are you sure you want to change the base?

[REFACTOR] 마이페이지 리팩토링 #357

Uh oh!

Conversation

dongx0915 commented Jun 16, 2024

📌 개요

✨ 작업 내용

✨ PR 포인트

📸 스크린샷/동영상

Uh oh!

unam98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeeha left a comment

Choose a reason for hiding this comment

Uh oh!

leeeha Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leeeha Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

leeeha Jun 19, 2024 •

edited

Loading

leeeha Jun 26, 2024 •

edited

Loading