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

경북대 Android_권성찬 5주차 과제 step 3 #79

Open
wants to merge 41 commits into
base: ksc1008
Choose a base branch
from

Conversation

ksc1008
Copy link

@ksc1008 ksc1008 commented Jul 26, 2024

  • Step 1 리팩토링 작업을 하다 보니, 프로젝트에 Step 3 까지 적용되는 요구사항을 거의 다 만족하고 있다는 것을 발견했습니다.
    스텝 별로 구분하는게 크게 의미가 없을 것 같아, Step1에서 일부 미비한 데이터 바인딩만 마저 적용한 채 바로 Step 3로 제출하겠습니다.
  • 원래 과제 중 LiveData를 프로젝트에 적용하는 것이 포함되어 있었습니다. 하지만 이전부터 LiveData를 활용해보았고, 특강 때 도메인 로직에선 LiveData보단 Flow를 활용하는 것이 더 적절하고, 코틀린 표준인 Flow가 편리한 기능들을 많이 내장하고 있어 확장성이 좋다고 배워 이번 주간에는 LiveData 대신 Flow를 활용해 보았습니다. 이러한 방식이 과제 요구사항과 맞지 않아 적절하지 않다면 다시 LiveData로 되돌리겠습니다.

이번 주간에 프로젝트를 대대적으로 리팩토링하면서 앱의 아키텍처가 크게 변경되었습니다. 안드로이드 권장 아키텍처에 맞춰 레이어 단위로 구분하고, MVVM의 제약사항에 맞춰 레이어간 데이터를 전달하는 방식을 일부 수정하였습니다.

아래는 간략히 도식화한 아키텍처 구조도입니다.
도메인 레이어에서 레포지토리에 접근하는 중간자로 Use Case를 사용해보려 했었는데, 아직 구현하지는 못했습니다.

Map App Architecture_2

코드를 작성하면서 어려웠던 점

과제에 Flow를 적용함에 있어 어려움이 있었습니다.

Room 같은 경우 내부적으로 Cold Flow를 반환하는 함수가 내장되어 있어 이를 ViewModel에 표출시키면 알아서 잘 작동했습니다.
하지만 나머지 레포지토리에서 이와 같은 패턴으로 ViewModel에 노출할 Flow를 만들어 보았는데, 생각만큼 잘 작동하지가 않았습니다.

결국 레포지토리에 HotFlow인 sharedFlow를 내장하는 방식으로 나머지를 구현하였습니다.

아직은 Flow가 정확히 작동하는 방식에 대한 이해가 부족한 것 같습니다.


카카오 맵 API는 독자적인 Lifecycle을 가지고, 서비스의 대부분의 상태가 뷰 내에 숨겨져 있어 MVVM을 적용하는게 상당히 까다로웠습니다.

중점적으로 리뷰해주셨으면 하는 부분

  • 현재 프로젝트 내에 LiveData와 Flow가 혼재되어 있는 상황입니다. 뷰모델에서 생성하고 사용되는 LIveData는 StateFlow로 변환하지 않고 놔뒀는데, 전부 Flow로 바꾸는게 옳을지 궁금합니다.

  • 프로젝트를 크게 개편하면서 우여곡절이 있었고, 레포지토리에 Flow 적용하는 부분 같은 경우에는 버그가 발생하지 않게끔만 구현한 상태입니다. 많은 부분이 미흡할텐데 레포지토리 패턴과 Flow 적용을 중점으로 많은 피드백 부탁드립니다.

ksc1008 added 30 commits July 27, 2024 16:53
--BREAKING CHANGE--
프로젝트의 패키지 구조를 클린 아키텍처에 맞춰 개편하였습니다.
Copy link

@mkSpace mkSpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 주차도 수고 많으셨습니다!

우선 Hilt 적용 및 테스트까지 잘 구현해주셨어요! 매 주차마다 다양한 도전을 하시는 모습이 대단합니다!

다양한 기술을 한 번에 적용하기엔 많이 힘들텐데 그 중에서도 Coroutine은 정말 많이 사용해봐야 체득되는것 같습니다.

현재 성찬님이 쓰시는 방법도 좋은데 아직은 Coroutine에 대한 학습이 조금 미흡한 부분이 보이네요. 그 점을 중점적으로 리뷰를 남겼으니 확인하시고 의견 남겨주세요~

Flow와 LiveData에 대해서 어떻게 사용해야 할 지에 대해 말씀해주셨는데 이는 프로젝트마다 모두 상이합니다. 제가 여러 개발자와 그간에 의논해 본 바 두 가지의 의견이 있었는데요.
이는 ViewModel 내에서는 LiveData를 쓰자! vs 어차피 Repository 단 부터는 Flow를 사용하는데 굳이 LiveData로 변환해야 하냐! 모두 다 Flow를 쓰자!
라는 의견인데요. 이는 매우 주관적인거라 본인만의 기준을 가지고 잘 사용하시면 되겠습니다. (저는 후자의 의견입니다 ㅎㅎ)

지금 Flow를 쓰는게 조금 어색할 수 있는데 그 이유는 Flow를 아직 활용하는 방법을 찾지 못해서 인 것 같아요. 그 이유는 잘 찾아보시면 Room 또한 데이터를 Flow로 내려받을 수 있는 기능을 제공합니다. 그래서 DB에서 부터 시작해 DB -> Repository -> ViewModel -> View 로 흐르는 Stream을 Flow로 연결해서 View binding을 적용할 수 있는데 이 구조를 설계하시면 Flow를 거의 극한으로 사용하시게 될 겁니다. 리뷰에서도 남겨 놓았는데 저는 SharedPrefereces를 핸들링 하는 객체를 하나 만들어서 그 안에서 특정 KEY를 갖는 데이터의 변화가 있을 때 해당 변화를 일으킨 data의 Flow를 각각 만들어서 Application Scope 내에서 접근할 수 있도록 하는 편입니다.

그리고 Remote Call은 단발성이란 것에 유념하셔야 합니다. 한 번의 실행만 이뤄지기에 Flow로 굳이 묶을 필요 없이 단발성의 Remote Call 이후 DB에 해당 내용을 저장하면 Room의 Dao에서 해당 테이블의 변경이 있을 때마다 Room이 최신 item을 offer 해주는 Flow를 활용해 Stream 구조를 만들어 View를 Update하는게 정석적입니다.

제가 코루틴을 배웠던 블로그 소개해드리니 Deep-Dive 해보시고 코루틴 마스터 하시길 빌겠습니다! ㅎㅎ (https://myungpyo.medium.com/reading-coroutine-official-guide-thoroughly-part-0-20176d431e9d)

이대로 Keep going 하시죠!

이러한 많은 도전이 언젠간 큰 결실로 다가올거라 생각됩니다!

Comment on lines 44 to 49
fun provideSearchKeywordDB(
@ApplicationContext context: Context
): SearchKeywordDB {
return Room.databaseBuilder(context, SearchKeywordDB::class.java, "MapSearch2")
.fallbackToDestructiveMigration().build()
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가지로 Module을 다른 파일로 분리해주시고 Database도 하나의 인스턴스를 의존 주입해서 분리하시면 더 깔끔할 것 같네요.
추후 Dao가 늘어날 것을 대비해 확장성 있는 코드를 작성해 보심 좋을 것 같아요

Comment on lines +15 to +17
override fun areItemsTheSame(oldItem: String, newItem: String): Boolean = oldItem == newItem

override fun areContentsTheSame(oldItem: String, newItem: String): Boolean = true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사실 이 부분은 의미가 없는 DiffCallback 입니다.
아직은 Diff Callback이 왜 필요한지 모르셔서 이렇게 구현하셨을 텐데 추후에 학습해보세요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 이부분은 차차 학습해보겠습니다! 간단하게 리사이클러뷰 애니메이션을 적용하기 위해 ListAdapter를 적용하던 과정에서, 파라미터로 요구하는 Diff Callback을 구현한 것입니다. 더 나은 사용법이 있는지 6주차 진행하면서 알아보겠습니다.

@mkSpace
Copy link

mkSpace commented Jul 28, 2024

그리고 Coroutine을 잘 사용한 프로젝트가 있다면 한 번 확인해보시는게 좋겠네요. 최신 레포들은 Hilt, Coroutine을 거의 다 사용하기에 star 높은 레포 하나 선택하셔서 공부해보셔도 좋을거라 생각됩니다. 한 번만 제대로 잡히면 다음은 금방입니다!

@ksc1008
Copy link
Author

ksc1008 commented Jul 29, 2024

언제나 상세한 피드백 남겨주셔서 감사합니다! 짚어주신 부분들 수정하여 푸시하였습니다.

피드백 주신 부분 중 추가로 궁금한 점 몇가지만 남겨드립니다.

전반적으로 빌드 객체를 싱글톤으로 의존 주입한다는 대목이 이해가 잘 가지 않습니다.


추후 확장을 위해 RetrofitBuilder를 통한 Retrofit Build 인스턴스를 싱글톤으로 의존 주입해주시면 더 깔끔할 것 같네요!

Retrofit.Builder()
            .baseUrl(BuildConfig.KAKAO_API_URL)
            .addConverterFactory(GsonConverterFactory.create())
            .build()
            .create(KakaoSearchRetrofitService::class.java)

위 코드 대신

Retrofit.Builder()
            .baseUrl(BuildConfig.KAKAO_API_URL)
            .addConverterFactory(GsonConverterFactory.create())
            .build()

코드를 적용해서 서비스는 DataSource 내에서 생성하라는 의미인가요?

아니면 Retrofit.Builder() 를 싱글톤으로 따로 분리하고, 서비스 객체를 싱글톤으로 만들 때 의존 주입하라는 의미일까요?


Database도 하나의 인스턴스를 의존 주입해서 분리하시면 더 깔끔할 것 같네요.

이부분도 마찬가지로 이해가 잘 가지 않습니다...

현재 Room의 DB 빌더로 생성한 Database 객체가 아니라면, 어떤 인스턴스를 분리해야 하는지 모르겠습니다.
아니면 DB와 관련된 기능 자체를 인터페이스로 추상화하여 주입하라는 의미인까요?

@ksc1008 ksc1008 requested a review from mkSpace July 29, 2024 10:10
@mkSpace
Copy link

mkSpace commented Aug 3, 2024

언제나 상세한 피드백 남겨주셔서 감사합니다! 짚어주신 부분들 수정하여 푸시하였습니다.

피드백 주신 부분 중 추가로 궁금한 점 몇가지만 남겨드립니다.

전반적으로 빌드 객체를 싱글톤으로 의존 주입한다는 대목이 이해가 잘 가지 않습니다.

추후 확장을 위해 RetrofitBuilder를 통한 Retrofit Build 인스턴스를 싱글톤으로 의존 주입해주시면 더 깔끔할 것 같네요!

Retrofit.Builder()
            .baseUrl(BuildConfig.KAKAO_API_URL)
            .addConverterFactory(GsonConverterFactory.create())
            .build()
            .create(KakaoSearchRetrofitService::class.java)

위 코드 대신

Retrofit.Builder()
            .baseUrl(BuildConfig.KAKAO_API_URL)
            .addConverterFactory(GsonConverterFactory.create())
            .build()

코드를 적용해서 서비스는 DataSource 내에서 생성하라는 의미인가요?

아니면 Retrofit.Builder() 를 싱글톤으로 따로 분리하고, 서비스 객체를 싱글톤으로 만들 때 의존 주입하라는 의미일까요?

Database도 하나의 인스턴스를 의존 주입해서 분리하시면 더 깔끔할 것 같네요.

이부분도 마찬가지로 이해가 잘 가지 않습니다...

현재 Room의 DB 빌더로 생성한 Database 객체가 아니라면, 어떤 인스턴스를 분리해야 하는지 모르겠습니다. 아니면 DB와 관련된 기능 자체를 인터페이스로 추상화하여 주입하라는 의미인까요?

RetrofitBuilder를 통해서 Build를 하면 Retrofit 인스턴스를 생성하잖아요. 이게 굳이 애플리케이션 내에 여러개 존재할 필요가 없으니 이 Retrofit 인스턴스를 싱글톤하게 의존주입해서 사용하시라는 말이었어요. 이렇게 주입한 Retorift을 가지고 RetrofitService들을 생성해 내면 되겠습니다.

더불어 DB도 마찬가지로 Custom Database를 싱글톤하게 의존 주입한 후 이렇게 주입한 database를 주입받아 dao들을 의존 주입하시는걸 말씀드린거였습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants