Skip to content
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

Refactor/253 Navigation 2.8.x Safe Args 적용 #313

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from
Open

Conversation

HamBP
Copy link
Member

@HamBP HamBP commented Oct 17, 2024

Issue

코멘트

작업 내용

작업하면서 몇 가지 리팩토링 더 작업했어

  • Screen에서 사용하는 파라미터 이름 onClickXXX -> navigateToXXX로 변경 : 4c2c3a8
    • onClickXXX에 넘겨줘야 하는 람다를 알기 위해서는 내부 동작까지 알아야 하더라구. navigateXXX라고 하면 더 추상화가 잘 되는 거 같아서 변경했어
  • modifier 적용 위치를 NavHost로 이동 : 299d5f3
    • 상위 컴포넌트에서 한 번에 처리하면 전체 코드 양이 줄어 들기도 하고, modifier를 여러 번 작성하면 중첩 적용될 우려도 있는 거 같아서 변경했어
  • navigation 편의를 위해 만든 XXXScreen 확장 함수 이름을 camelCase로 변경 : 3e3c081
    • 기존에 PascalCase를 썼던 이유는 Composable 함수 느낌으로 사용하려고 했었어. 그런데 IDE에서 밑줄이 생기기도 하고 XXXSceen이랑 이름이 같아서 생기는 불편함이 조금 있더라구. 그래서 변경했는데 Now in Android에서 사용하는 네이밍으로 변경했어
  • NavController를 다시 파라미터로 넘겨 줬어. 기존에 넘겨줬던 게 navigate(route: String)인데 마이그레이션 하려면 navigate(route: T)가 필요해서 결국 다 바꿔야 하더라구. 그래서 차라리 navController를 넘겼어. 보통 재사용성이나 독립적 테스트 때문에 안 넘기는 거 같은데, 이 확장함수는 그런 목적은 아니어서 넘겨줘도 될 것 같다고 생각했어

추가 TMI

중요한 몇 가지 navigation 로직에 대해서만 테스트 코드를 작성해 보려고 했는데 두 가지 문제가 있더라구.

  • 같은 테스트 코드로 마이그레이션 전과 후를 모두 테스트할 수 없었어. 이전에는 route 문자열 값으로 현재 화면을 판별하지만 이후에는 KClass로 현재 화면을 판별하기 때문에 테스트가 깨지겠더라구.
  • 테스트 하려면 데이터 로드 또는 더미 데이터가 필요해. 공연 화면에서 상세 화면으로 넘어 가려면 showId가 필요하고, ViewModel에 상태 값을 넣어줘야 하더라구. 물론 ViewModel은 ViewModel과 Screen을 분리하면 간단히 해결될 거 같긴 해.

테스트 방법

  • 대신 노션에 내비게이션 가능한 28 + @개 항목을 작성하고 테스트 해 봤어.
  • 특정 화면을 테스트하기 위해서는 해당 화면까지 실 데이터를 가지고 접근해야 하는데, 특정 화면만 테스트할 수 있는 환경을 만들어봐도 재밌을 듯

HamBP added 4 commits October 18, 2024 04:37
예를 들어 onClickShow 라는 람다를 넘겨주려면 이 화면에서 공연이 클릭 되었을 때 비즈니스 로직을 알아야 한다. 대신 navigateToXXX 라는 이름으로 변경해 어떤 람다를 넣어줘야 하는지 쉽게 알도록 했다.
@HamBP HamBP added the refactor 기능은 그대로인데, 코드만 바꾸는 경우 label Oct 17, 2024
@HamBP HamBP self-assigned this Oct 17, 2024
@HamBP HamBP marked this pull request as draft October 17, 2024 20:14
Copy link

Test Results

 9 files   9 suites   0s ⏱️
 8 tests  8 ✅ 0 💤 0 ❌
12 runs  12 ✅ 0 💤 0 ❌

Results for commit 4c2c3a8.

@HamBP HamBP requested a review from mangbaam October 26, 2024 10:25
@HamBP HamBP marked this pull request as ready for review October 26, 2024 10:25
@mangbaam
Copy link
Member

이 작업 이후에 화면들도 더 추가되고, 컨플릭도 발생할 거 같아서
이번 배포 이후에 살펴보면서 파악한 후에 이 PR 이후 진행됐던 작업들에 반영해볼게

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor 기능은 그대로인데, 코드만 바꾸는 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation SafeArgs 적용
2 participants