[refact/all]: App 추가화면 로직 수정 #158

kez-lab · 2024-03-05T16:02:45Z

간단한 리팩터링인데 꽤나 고민해보고 고쳐보았습니다.

Activity ViewModel 통합
State data 가공을 ViewModel로 옮김
updateState의 경우 ViewModel에서만 가능하게 변경하여 상태관리는 ViewModel의 책임으로 옮김

두 분 다 보시고 어떤 식으로 우리가 좋은 코드를 만들어나가야할지 고민해보면 좋을 것 같아요~!!
보시고 이해 안가는 부분이 있거나 오류가 있는지 알려주세욥

jihyun0v0 · 2024-03-06T20:12:45Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddActivity.kt

+    private fun collectState() {
+        viewModel.state.flowWithLifecycle(lifecycle)
+            .onEach { binding.btAppSelection.isEnabled = it.appSelectionList.isNotEmpty() }
+            .launchIn(lifecycleScope)


이건 무슨 기능을 하는 코드인가요?

UDA 구조 -> 즉 단방향 데이터 통신을 통해 현재 View의 상태를 최신화 할 수 있도록 ViewModel에서 관리하는 State를 참조하도록 하는 코드입니다.
안드로이드에서는 MVI패턴이라고 불려요

jihyun0v0 · 2024-03-06T20:13:46Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddActivity.kt

-        }.launchIn(lifecycleScope)
-    }
+            btAppSelection.setOnClickListener { handleNextClicked() }
+            ivBack.setOnClickListener { finish() }


뒤로가기 버튼 눌렀을 때 이 액티비티를 종료하는 코드인 거 맞나요?

넹 정답~!!

jihyun0v0 · 2024-03-06T20:14:57Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddActivity.kt

완전 깔끔해졌다.!! 고마워 의진아 ^_<

jihyun0v0 · 2024-03-06T20:23:16Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

    private val _effect = MutableSharedFlow<AppAddEffect>()
    val effect = _effect.asSharedFlow()


이 effect는 어디서 사용되는 건가요??

해당 effect는 ViewModel -> View로 단방향 이벤트를 쏘기 위해서 사용됩니다.
단발성으로 이루어져야하는 동작에 쓰이며 Event의 경우에는 state보다 더욱 귀중한 자원으로 다뤄야합니다.(이벤트는 한번 발신 후 수신하지 못하면 유실되기 때문)

SharedFlow에 reply를 걸어놓아서 백그라운드 상황의 이벤트에도 대응을 하고싶네요

jihyun0v0 · 2024-03-06T20:57:43Z

...lenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/appselection/AppSelectionFragment.kt

+        appSelectionList.filter {
+            val packageName = it.packageName
+            val appName = context?.getAppNameFromPackageName(packageName).orEmpty()
+            !packageName.startsWith(HMH_PACKAGE_NAME) && appName.contains(filter)


여기서 filter는 무슨 역할을 하는 건가요??

검색기능이지 않을까싶네요? 이건 이전에 있던 코드였어서

아 새로 추가된 거로 나와서 새로운 기능인줄 알았습니다 허헣

jihyun0v0

훨씬 깔끔해지고 통일된 것 같습니다. 잘 배워갑니다. 수고하셨습니다~~

kez-lab · 2024-03-07T02:13:04Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

    val goalTime = goalHour + goalMin
+    val appSelectionList = installApps.map { AppSelectionModel(it, selectedApps.contains(it)) }
 }


이거 시간 복잡도가 조금 꺼림칙합니다 더 좋은방법 있다면 추천?

어떤 부분에서 시간 복잡도 걱정이 된다는 건지 자세하게 알 수 있을까용?

map으로 한번 반복시키고 내부에서 contains로 반복시키면 O^2가 됩니당

kez-lab · 2024-03-07T02:13:28Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

    private val _effect = MutableSharedFlow<AppAddEffect>()
    val effect = _effect.asSharedFlow()


SharedFlow에 reply를 걸어놓아서 백그라운드 상황의 이벤트에도 대응을 하고싶네요

kez-lab · 2024-03-07T02:13:44Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

+    private fun updateState(transform: AppAddState.() -> AppAddState) {
        val currentState = state.value
        val newState = currentState.transform()
        _state.value = newState
    }
+


상태 업데이트는 ViewModel에서만!

kez-lab · 2024-03-07T02:14:20Z

...llenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/appselection/AppSelectionAdapter.kt

+            setListeners(appSelectionModel)
        }

-        private fun initAppSelectionListener(packageName: String) {
-            binding.root.setOnClickListener {
-                if (adapterPosition != RecyclerView.NO_POSITION) {
-                    if (binding.cbApp.isChecked) {
-                        binding.cbApp.isChecked = false
-                        onAppCheckboxUnClicked(packageName)
-                        checkBoxStatus.put(adapterPosition, false)
+        private fun setListeners(appSelectionModel: AppSelectionModel) {


제가 만들었지만 이름이 너무 그지같아요

check된 값에 따라서 클릭리스너가 보여지는 걸 업데이트 하는 함수니까...
updateCheckedApp아니면..
updateCheckedAppListner 등등 .. 이런 식..?

좋은 것 같아요

kez-lab · 2024-03-07T02:15:10Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/time/SetGoalTimeFragment.kt

+            viewModel.setGoalHour((newTime * 60 * 60 * 1000).toLong())
        }
        binding.npUseTimeGoalMinute.setOnValueChangedListener { _, _, newTime ->
-            activityViewModel.updateState {
-                copy(goalMin = (newTime * 60 * 1000).toLong())
-            }
+            viewModel.setGoalMin((newTime * 60 * 1000).toLong())


요것도 좀 확장으로 빼고싶네요

kangyuri1114

구웃 좋은 리팩토링 코드 잘 보고 갑니둥

kangyuri1114 · 2024-03-07T04:29:56Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

    val goalTime = goalHour + goalMin
+    val appSelectionList = installApps.map { AppSelectionModel(it, selectedApps.contains(it)) }
 }


어떤 부분에서 시간 복잡도 걱정이 된다는 건지 자세하게 알 수 있을까용?

kangyuri1114 · 2024-03-07T04:31:38Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt


 data class AppAddState(
-    val selectedApp: List<String> = listOf(),
+    val installApps: List<String> = emptyList(),


별 거는 아닌데
이 변수도 installedApp으로 하면 어떨까유

아 그리고 전부터 궁금했는데 viewModel에서 우리가 상태 업데이트할 때
state data class나 event object들을 따로 파일로 만들지 않고 viewModel에서 같이 생성하는 이유가
굳이 파일을 만들 필요 없음 + viewModel 파일의 가독성
뿐인것 맞을까용??

넵넵 맞습니다~!!
이름 아주 좋네요

kangyuri1114 · 2024-03-07T04:34:27Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

+    private fun updateState(transform: AppAddState.() -> AppAddState) {
        val currentState = state.value
        val newState = currentState.transform()
        _state.value = newState
    }
+


kangyuri1114 · 2024-03-07T04:42:31Z

feature/challenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/AppAddViewModel.kt

+    fun setGoalHour(goalHour: Long) {
+        updateState {
+            copy(goalHour = goalHour)
+        }
+    }
+
+    fun setGoalMin(goalMin: Long) {
+        updateState {
+            copy(goalMin = goalMin)
+        }
+    }


시간이랑 분을 따로 업데이트 하는 거 보다는 한 함수로 합치는건 어떨까요??
시간따로 분따로 받을 일이 있을까요???

그 리스너가 분리되어있습니다 입력되는 뷰가 달라서요!

아하! 넹~~

kangyuri1114 · 2024-03-07T04:48:26Z

...llenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/appselection/AppSelectionAdapter.kt

+            setListeners(appSelectionModel)
        }

-        private fun initAppSelectionListener(packageName: String) {
-            binding.root.setOnClickListener {
-                if (adapterPosition != RecyclerView.NO_POSITION) {
-                    if (binding.cbApp.isChecked) {
-                        binding.cbApp.isChecked = false
-                        onAppCheckboxUnClicked(packageName)
-                        checkBoxStatus.put(adapterPosition, false)
+        private fun setListeners(appSelectionModel: AppSelectionModel) {


check된 값에 따라서 클릭리스너가 보여지는 걸 업데이트 하는 함수니까...
updateCheckedApp아니면..
updateCheckedAppListner 등등 .. 이런 식..?

kangyuri1114 · 2024-03-07T05:03:42Z

...lenge/src/main/java/com/hmh/hamyeonham/challenge/appadd/appselection/AppSelectionFragment.kt

    private val binding by viewBinding(FrargmentAppSelectionBinding::bind)
-    private val viewModel by viewModels<AppSelectionViewModel>()
-    private val activityViewModel by activityViewModels<AppAddViewModel>()
+    private val viewModel by activityViewModels<AppAddViewModel>()


AppSelectionViewModel의 프래그먼트 뷰모델 코드를 모두 액티비티의 AppAddViewModel로 옮긴 것 같은데 맞을까요? 프래그먼트 뷰모델에서 하는 일이 없는 것 같은데 그러면 AppSelectionViewModel 파일 지우는 거 어떨까욤

AppSelectionViewModel 요거 지워야할 것 같네요 굳!!

[refact/all]: App 추가화면 로직 수정

[refact]: App Add 로직 수정

cd40dcf

kez-lab added 🐱의진 의진이 작업 🔎refact 내부 로직은 변경하지 않고 기존의 코드를 개선하는 리팩토링 labels Mar 5, 2024

kez-lab self-assigned this Mar 5, 2024

kez-lab requested a review from a team as a code owner March 5, 2024 16:02

jihyun0v0 reviewed Mar 6, 2024

View reviewed changes

kez-lab commented Mar 7, 2024

View reviewed changes

kangyuri1114 approved these changes Mar 7, 2024

View reviewed changes

[refact]: PR 대응

fb4500e

kez-lab merged commit f354f29 into develop Mar 7, 2024
1 check passed

kez-lab deleted the refact/all branch March 7, 2024 06:09

kez-lab added a commit that referenced this pull request Nov 20, 2024

Merge pull request #158 from Team-HMH/refact/all

7ba61ac

[refact/all]: App 추가화면 로직 수정

		private val _effect = MutableSharedFlow<AppAddEffect>()
		val effect = _effect.asSharedFlow()

[refact/all]: App 추가화면 로직 수정 #158

[refact/all]: App 추가화면 로직 수정 #158

Conversation

kez-lab commented Mar 5, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihyun0v0 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

Choose a reason for hiding this comment

kangyuri1114 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

kez-lab Mar 7, 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

kez-lab Mar 7, 2024 •

edited

Loading