-
Notifications
You must be signed in to change notification settings - Fork 32
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
[강원대 안드로이드 주민철] 5주차 과제 스텝2 #82
base: joominchul
Are you sure you want to change the base?
[강원대 안드로이드 주민철] 5주차 과제 스텝2 #82
Conversation
Room으로 변경
뷰모델에 레트로핏, 데이터베이스 DAO, sharedpreferences 의존성 주입
추가로 다른 함수들 리팩토링함.
추가로 맵액티비티 안 쓰는 import 제거함
힐트를 적용함에 따라 기존 테스트 코드가 작동을 안 함.
* Rename[]: 추가 4주차 코드 * Rename[]: 추가 4주차 코드 * Refactor[]: 추가 콜백 함수 onPlaceClicked * Refactor[]: 변경 warnings 사항 * Refactor[]: 변경 warnings 사항 * Refactor[]: 변경 서치액티비티를 서치프래그먼트로 * Test[]: 변경 서치액티비티를 서치프래그먼트로 * Fix[]: 수정 db.close 순서 * Refactor[]: 수정 warnings
* Rename[]: 추가 4주차 코드 * Rename[]: 추가 4주차 코드 * Refactor[]: 추가 콜백 함수 onPlaceClicked * Refactor[]: 변경 warnings 사항 * Refactor[]: 변경 warnings 사항 * Refactor[]: 변경 서치액티비티를 서치프래그먼트로 * Test[]: 변경 서치액티비티를 서치프래그먼트로 * Fix[]: 수정 db.close 순서 * Refactor[]: 수정 warnings * Docs[README.md]: 생성 리드미 내용 * Refactor[]: 변경 데이터베이스 Room으로 변경 * Comment[MainViewModel]: 삭제 주석 * Refactor[]: 추가 힐트 뷰모델에 레트로핏, 데이터베이스 DAO, sharedpreferences 의존성 주입 * Refactor[]: 수정 warnings * Comment[RetrofitData]: 삭제 주석 * Refactor[SearchWord]: 변경 어노테이션 상수 * Refactor[Retrofit]: 분리 레트로핏 서비스 부분 서비스 부분을 모듈로 분리시킴 * Fix[FunTest]: 수정 FunTest 에러 mockk을 사용해 모델 구성
리베이스 한 번 해주세요! |
Room으로 변경
뷰모델에 레트로핏, 데이터베이스 DAO, sharedpreferences 의존성 주입
추가로 다른 함수들 리팩토링함.
추가로 맵액티비티 안 쓰는 import 제거함
힐트를 적용함에 따라 기존 테스트 코드가 작동을 안 함.
…-joominchul-step2 # Conflicts: # app/src/main/java/campus/tech/kakao/map/Module.kt # app/src/main/java/campus/tech/kakao/map/dto/SearchWord.kt # app/src/main/java/campus/tech/kakao/map/url/RetrofitData.kt # app/src/test/java/campus/tech/kakao/map/FunTest.kt
코루틴 사용으로 인한 에러 수정
완료했습니다~! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이번주차도 수고 많으셨습니다!
우선은 다음 스텝 진행하시면서 Android Repository Pattern에 대해서 학습해보셨으면 좋겠습니다. 현재 ViewModel이 하는 역할이 매우 크고 로직들이 많이 파편화되어있습니다.
더불어서 ViewModel과 View의 역할에 대해서 더욱 학습이 필요합니다. 현재 비즈니스 로직들이 View에 많이 담겨있습니다. View는 그리는것에만 최선을 다해야 합니다.
Coroutine에 대한 이해도 조금 떨어져보입니다. 제가 코루틴을 배운 블로그 소개해드리니 Deep-Dive 해보시죠!
사실 많은 기술 스택을 하나의 프로젝트만 해서 모두 체득화하기는 많이 힘들거에요. Coroutine + Hilt가 잘 적용된 레포지토리 하나 찾아서 클론 코딩해보시는걸 추천드립니다.
결국 한 번만 잘 이해하고 잡아놓으면 그 다음은 편합니다!
나머지는 리뷰 남겨드리니 확인해보시고 의견 달아주세요!
추천드리는 블로그 입니다!(https://myungpyo.medium.com/reading-coroutine-official-guide-thoroughly-part-0-20176d431e9d)
viewModelScope.launch(Dispatchers.IO) { | ||
searchWordDao.delete(word.name, word.address, word.type) | ||
loadWord() | ||
}.join() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
join
을 쓰신 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에는 당연히 조인은 안 썼습니다만, 그렇게 하니 addWord 함수에서 삭제를 하고, 삽입을 해야 하는데, 삽입을 하고 삭제가 되더군요. 아무래도 비동기 처리를 하다보니 순차적으로 작동이 되지 않는 것 같아, 조인을 사용해 삭제가 먼저 처리되도록 하였습니다.
class MainViewModel @Inject constructor( | ||
application: Application, private val retrofitData: RetrofitData, | ||
private val searchWordDao: SearchWordDao, private val mapPosition: MapPositionPreferences | ||
) : AndroidViewModel(application) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Android Repositroy Pattern에 대해 아시나요? 현재 Repository의 역할을 겸하고 있는것으로 보이네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아직 잘 모르고, 아마 데이터베이스와 관련된 함수들을 관리하지 않을까 생각하고 있습니다.
import javax.inject.Inject | ||
|
||
class RetrofitData @Inject constructor(private val retrofitService: RetrofitService) { | ||
private val _documents = MutableLiveData<List<Document>>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LiveData는 사실 ViewModel까지만 사용하는게 원칙적입니다. Coroutine의 Flow를 활용해보세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flow를 사용하려 시도하였으나, 아직까진 수월하지 않네요..ㅠㅠ
fun searchPlace(query: String): Flow<List<Document>> = flow { try { val response = retrofitService.requestPlaces(AUTHORIZATION, query).execute() if (response.isSuccessful) { val documentList = response.body()?.documents?.map { document -> Document( document.placeName, document.categoryGroupName, document.addressName, document.longitude, document.latitude ) } ?: emptyList() emit(documentList) } else { // 에러 처리 Log.e("RetrofitData", "else Error: ${response.code()}") emit(emptyList()) } } catch (e: Exception) { // 에러 처리 Log.e("RetrofitData", "catch Error: ${e.message}") emit(emptyList()) } }
기존의 RetrofitData 클래스의 searchPlace 함수 대신 위의 함수를 만들었으나, 계속해서 catch 에러가 뜹니다. 메시지를 확인하면 null로만 나오니 이게 무슨 의미인지도 모르겠네요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
댓글로는 코드를 보시기 어려우실 것 같아, 주석 처리하여 push 하였습니다.
}.join() | ||
} | ||
|
||
suspend fun loadWord(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suspend fun
일 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코루틴 내부에서 호출하는 함수는 suspend 함수여야 된다고 알고 있어서 사용을 했습니다. 하지만 실제로 suspend를 지워도 잘 작동하네요..;;
@AndroidEntryPoint | ||
class MapActivity : AppCompatActivity() { | ||
private var map: KakaoMap? = null | ||
private lateinit var model: MainViewModel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by viewModels
를 써서 providing 받아보세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ViewModelProvider 대신 사용하란 말씀이신가요?
return Retrofit.Builder() | ||
.baseUrl(UrlContract.BASE_URL) | ||
.addConverterFactory(GsonConverterFactory.create()) | ||
.build() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrofit Builder를 통해 생성하는 Retrofit을 따로 주입하는게 확장성엔 더 좋겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
분리해 보았습니다.
fun provideDatabase(@ApplicationContext context: Context): SearchWordDatabase{ | ||
return Room.databaseBuilder(context, SearchWordDatabase::class.java, SearchWordContract.DB_NAME) | ||
.build() | ||
} | ||
|
||
@Provides | ||
@Singleton | ||
fun provideDao(database: SearchWordDatabase): SearchWordDao { | ||
return database.searchWordDao() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
따로 DatabaseModule 정도로 분리할 수 있겠네요.
CoroutineScope(Dispatchers.IO).launch{ | ||
model.loadWord() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 작성하면 이 Coroutine이 무한정 실행된다고 가정할 때 언제 종료될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프래그먼트의 생명주기를 따라 종료될 가능성이 높다고 생각이 됩니다만... 찾아보니 프래그먼트에서는 코루틴스코프 대신 lifecycleScope를 주로 사용하는 것 같습니다. 그래서 라이프사이클스코프로 변경해 보았습니다.
if (query.isEmpty()){ | ||
searchBinding.noSearchResult.visibility = View.VISIBLE | ||
searchBinding.searchResultRecyclerView.visibility = View.GONE | ||
}else{ | ||
searchBinding.noSearchResult.visibility = View.GONE | ||
searchBinding.searchResultRecyclerView.visibility = View.VISIBLE | ||
model.searchLocalAPI(query) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이렇게 판단하는 로직도 ViewModel에 있는게 베스트입니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비즈니스 로직들을 뷰모델로 옮겨보았습니다. 확인 부탁드립니다.
override fun areItemsTheSame(oldItem: SearchWord, newItem: SearchWord): Boolean { | ||
return oldItem === newItem | ||
} | ||
|
||
override fun areContentsTheSame(oldItem: SearchWord, newItem: SearchWord): Boolean { | ||
return oldItem == newItem | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 의미 없는 Diff Callback입니다. 아직은 필요성에 대해 인지하지 못할 수 있는데 나중에 payload를 통한 bind도 학습해보세요!
의미가 없는 이유는 areItemsTheSame이 True이고 areContentsTheSame이 false여야 변경된 부분만 트래킹 수 있기 때문입니다. 지금은 areItemsTheSame이 true인데 areContentsTheSame이 false일 수는 없겠네요
값을 뷰모델에서 라이브데이터로 받아옴
모듈을 레트로핏 모듈과 데이터 모듈로 분리함
데이터바인딩을 위해 placeName과 addressName의 private를 지우고, 대신 lateinit 속성을 추가함
Retrofit을 따로 주입하도록 분리
라이브데이터 옵저브와 관련된 코드들과 doOnTextChanged 함수와 관련된 코드를 뷰모델로 이동시킴
라이브데이터 옵저브와 관련된 코드들과 doOnTextChanged 함수와 관련된 코드를 뷰모델로 이동시킴
어려웠던 부분
봐주셨으면 하는 부분