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

코드 리뷰에 있어서 질문 드리고 싶은 부분 정리 #74

Open
likppi10 opened this issue Aug 18, 2023 · 7 comments
Open

코드 리뷰에 있어서 질문 드리고 싶은 부분 정리 #74

likppi10 opened this issue Aug 18, 2023 · 7 comments
Assignees
Labels
documentation Improvements or additions to documentation refactoring Code restructure for better readability and efficiency

Comments

@likppi10
Copy link
Collaborator

likppi10 commented Aug 18, 2023

전체적으로 프로젝트의 구조는 now in android를 참고하여 모듈화를 진행하였습니다.

  1. 본 프로젝트는 di를 core/data 모듈에서 진행하고 있는데, app 모듈에서 di를 진행하는 것이 더 나은 방법인 것인지, 이대로도 괜찮은지 궁금합니다.(이석규)
    core/data/di

  2. ViewModel에서 관리 중인 UiState가 앱의 기능이 추가됨에 있어 파라미터들이 하나씩 추가되어, 하나의 UiState 자체가 비대해지는 문제가 발생하였는데, 이것을 어떤 기준, 방법을 통해서 조금이나마 분리하여, 나눌 수 있을지, 이대로도 괜찮은 지 궁금합니다.
    UiState 를 data class 로 선언하여 관리할때는 그 화면이 간단할때만 해야된다는걸 체감하게 되는 것 같슴니다..(이지훈, 이석규)
    HomeViewModel.kt
    BandalartBottomSheetViewModel.kt

  3. 표를 보여주는 HomScreen 컴포저블은 어느정도 함수의 모듈화가 이루어졌다고 생각하는데, BottomSheet를 이용하는 부분은 무수한 spacer와 image가 card에 둘러싸여 있는 등 날것의 컴포저블들이 상당 수가 BandalartBottomSheet 내부에 그대로 포함 되어있습니다. 바텀 시트를 보셨을 때, 어떻게 함수를 분리하면 좋을지, 생각하시는 나뉠수 있는 기준, 방법이 궁금합니다.(이석규)
    HomeScreen.kt
    BandalartChart.kt
    BandalartBottomSheet

  4. 앱에 대한 기능을 테스트해보면서 홈화면에서 불필요하게 많이 Recomposition이 수행되는걸 확인할 수 있었는데(표를 불러오는 과정에서 TopBar 에 반다라트라는 단어가 계속 깜빡거리는 등의..) Recomposition 을 현 상황에서 어떻게 최적화시킬수 있을지, 최적화 시킬 수 있는 부분이 존재하는지 궁금합니다! (이런 부분을 찾아내서 최적화 하는 것이 진정한 실력있는 개발자인데, 아직 개선시킬 수 있는 부분을 잘 찾지 못하겠습니다...)(이지훈)
    HomeScreen.kt
    HomeViewModel.kt

또한 뷰모델의 특정한 하나의 함수가 너무나도 많은 역할을 수행하고 있고, 이것이 좋지 못한 방법이라는 것을 인지하고 있어, 개선해보도록 하겠습니다..

코드를 확인해주셔서, 그리고 리뷰해주셔서 감사합니다!

@likppi10 likppi10 added the documentation Improvements or additions to documentation label Aug 18, 2023
@likppi10 likppi10 self-assigned this Aug 18, 2023
@easyhooon easyhooon changed the title 프로젝트를 보는 분들에게 여쭤보고 싶은 점 코드 리뷰에 있어서 질문 드리고 싶은 항목 정리 Aug 18, 2023
@easyhooon easyhooon self-assigned this Aug 18, 2023
@easyhooon easyhooon changed the title 코드 리뷰에 있어서 질문 드리고 싶은 항목 정리 코드 리뷰에 있어서 질문 드리고 싶은 부분 정리 Aug 18, 2023
@taehwandev
Copy link

  1. 본 프로젝트는 di를 core/data 모듈에서 진행하고 있는데, app 모듈에서 di를 진행하는 것이 더 나은 방법인 것인지, 이대로도 괜찮은지 궁금합니다.(이석규)

module에서 di를 한다는건 나중에 모듈을 붙여서 정의해둔 di를 붙이겠다는 의미이고, app에서 module을 정의하겠다는건 모듈에 상관 없이 di와 관련 없는 모듈을 정의하고, 이를 가져와 쓰겠다는 의미입니다.
어떤 방법이 더 적합한가?에 대한 답은 없어요. 어떤 이유로 이걸 적절하게 쓴건지가 중요하죠.

@taehwandev
Copy link

  1. ViewModel에서 관리 중인 UiState가 앱의 기능이 추가됨에 있어 파라미터들이 하나씩 추가되어, 하나의 UiState 자체가 비대해지는 문제가 발생하였는데, 이것을 어떤 기준, 방법을 통해서 조금이나마 분리하여, 나눌 수 있을지, 이대로도 괜찮은 지 궁금합니다.
    UiState 를 data class 로 선언하여 관리할때는 그 화면이 간단할때만 해야된다는걸 체감하게 되는 것 같슴니다..(이지훈, 이석규)

UiState를 왜 사용하나요? 지금 코드상 보면
단순히 특정 2개를 UiState루 묶어서 쓰고있고, copy를 지속하고 있습니다. 이걸 sealed interface로 나눠서 처리할수도 있어요. uiState는 화면이 복잡해지면 당연히 복잡해져요. ViewModel을 나누는것도 방법일거고, UiState를 sealed interface로 나누는것도 방법일거에요. 근데 우리는 상태를 관리해야하니 적절한 형태가 보이지는 않을것같고, 결국 view 단위 하나로 viewModel을 나누면 해결될 문제이긴 합니다.

@taehwandev
Copy link

  1. 표를 보여주는 HomScreen 컴포저블은 어느정도 함수의 모듈화가 이루어졌다고 생각하는데, BottomSheet를 이용하는 부분은 무수한 spacer와 image가 card에 둘러싸여 있는 등 날것의 컴포저블들이 상당 수가 BandalartBottomSheet 내부에 그대로 포함 되어있습니다. 바텀 시트를 보셨을 때, 어떻게 함수를 분리하면 좋을지, 생각하시는 나뉠수 있는 기준, 방법이 궁금합니다.(이석규)

BottomSheet의 용도에 따라 다른거라서 지금 코드가 복잡할수도 있지만 아닐수도 있어요.
좀 더 개선을 해본다고하면 BottomSheet의 기본 틀을 정하고, 그 안에 컨텐츠 부분(버튼을 포함)한 부분으로 분리할순 있을것고, UiState에 따라 차트용, 테이블용, 이미지용, 리스트용을 sealed interface로 분리해서 처리하는것도 가능하죠.

@taehwandev
Copy link

  1. 앱에 대한 기능을 테스트해보면서 홈화면에서 불필요하게 많이 Recomposition이 수행되는걸 확인할 수 있었는데(표를 불러오는 과정에서 TopBar 에 반다라트라는 단어가 계속 깜빡거리는 등의..) Recomposition 을 현 상황에서 어떻게 최적화시킬수 있을지, 최적화 시킬 수 있는 부분이 존재하는지 궁금합니다! (이런 부분을 찾아내서 최적화 하는 것이 진정한 실력있는 개발자인데, 아직 개선시킬 수 있는 부분을 잘 찾지 못하겠습니다...)(이지훈)

Recomposition이 뭘까요? 화면이 갱신되어지면 리컴포지션은 자연스럽게 일어나요. 그 말은 화면의 어느 부분이 수정되어지는 과정에서 recompose가 발생하고, 이때 데이터의 변화가 있으면 카운트가 올라갑니다. 매우 자연스러운것인데, 지금 코드상으론 uiState가 덩치가 크니, 어떤게 변하더라도, UiState 데이터 객체가 변경이 일어나면 화면 갱신이 일어나죠. 이건 당연한거라서 이걸 줄이려면 상태관리가 필수입니다.
상태 관리에 대한 2022년, 2023년 발표자료가 있어요. 이런 내용을 참고하셔서 상태관리가 왜 필요한지부터 학습하시면 좋아요.
화면에 시계하나가 변경되어도, 화면은 새로 그려집니다. 그 과정에 리컴포즈가 발생하고, 이때 데이터가 달라진걸 찾아서 갱신하는것이니, 그 갱신 기준을 잘 잡아주면 화면 갱신에 깜박거리는 데이터의 흐름도 없어지겠죠. 깜박 거린다는건 값이 "" 였다가 "a"의 값이 다시 채워지는것 때문에 발생하는거에요. 그럼 UiState 쪽에서 값을 잘못 전달되었을 가능성도 있는것이겠죠.

@easyhooon
Copy link
Collaborator

easyhooon commented Aug 21, 2023

@taehwandev 답변 감사드립니다! SpeakerDeck에 최근에 발표하신 What’s new in Android? 발표자료를 통해 말씀하신 2022년, 23년 발표자료가 무엇인지 찾았습니다. 꼭 확인해보도록 하겠습니다! 또한 UiState를 구현하는 방식을 data class 에서 sealed interface를 이용하는 방법으로 각 상태별로 어떻게 나눠야 좋을지 고민해보도록 하겠습니다! ㅎㅎ

@likppi10
Copy link
Collaborator Author

@taehwandev
세심한 답변에 감사드립니다! 바텀시트의 경우 기본틀을 정해서 영역을 틀을 다시 한번 정해보겠습니다! di 관련은 저도 지훈님이 이렇게 하는 것이 어떤가에 대해 제안 했을 때, domain이나 여타 모듈에서 따로 di를 정의하지 않는다는 점과 뿐만 아니라 di를 해당 모듈이 직접 정의하여 di까지 하는 것이 모듈 응집도도 가질 수 있지 않나라는 점이 근거를 가질 것이라 생각하여 동의했습니다. 제 지인들이 data 모듈에 있는 di를 생소하게 받아들이는 것에 의문이 들고, di를 개인의 근거를 가지고 배치해도 되는지의 당위성에 대해 한 번 의견을 여쭙고 싶었습니다. 다시 한 번 답변에 감사합니다. ㅎㅎ

@taehwandev
Copy link

@easyhooon @likppi10 NIA를 기반으로 나만의 코드를 만드시면 좋습니다! 두분이서 함께 하고 있다보니 설계에 대한 고민을 하실 수 있어 좋아보이고요. 근데 여기서 중요한건 이렇게 할려고 하는가. NIA도 솔직히 완전하다고 생각하진 않아요. 복잡한 구조이고, 뭐하러 이렇게 나눴을까하는 부분도 있고요. 이게 정답은 아니겠지만 공부하는덴 도움은 됩니다. 설계에서는 그래서 이게 정말 떨어지게 쉽게 만들어졌느냐도 중요할것이고, 가져다 쓰기 편할까도 중요한 부분이라서요.
지금 코드에서도 모든 UseCase... 가 엄청 많아 보이는데요. 실제로 이 UseCase는 안에 포워딩 하라고 있는것은 아니닙니다. Usecase 한개에 하나의 포워딩... 이게 유닛 테스트 효율이 올라갈까...? interface로 repository를 구성했는데 그럼 이것만 있어도 동일한거 아닌가? 나눠졌을때 정말 효율적일까? 이름으로부터 오는 장점이 있는가? 그럼 함수명 잘지어주면 같은거 아닌가? 등도 고민해볼법 합니다.

UseCase 구글 가이드상으론 단순 포워딩 할 때는 사용하지 않고, 두개의 Repository를 합치는 경우에 활용합니다.

이런것들도 고민이 필요하고, 사실 모든게 다 고민이 필요해요. 고민 없이 따라하는건 쉽지만, 이후부턴 전부다는 아니고, 하나둘 정도 궁금해하시면서 조금 더 고민해보면 좀 더 좋은 코드가 나올 수 있으실겁니다.(당연히 최고는 고민 없이 작성할 수 있어야 좋죠)

@easyhooon easyhooon added the refactoring Code restructure for better readability and efficiency label Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation refactoring Code restructure for better readability and efficiency
Projects
None yet
Development

No branches or pull requests

3 participants