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 박정훈 4주차 과제 Step2 #93

Open
wants to merge 14 commits into
base: pjhn
Choose a base branch
from

Conversation

Pjhn
Copy link

@Pjhn Pjhn commented Jul 19, 2024

테스트 코드 작성

  • UI 테스트 코드 작성 ( 검색 목록 화면, API 호출 화면 )
  • repository 테스트 코드

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

  • 테스트 과정에서 에러가 발생하여 과제 제출이 늦어졌습니다.
  • 아직 테스트 코드 작성이 익숙치 않은 것 같습니다.

코드 리뷰 시, 멘토님이 중점적으로 리뷰해줬으면 하는 부분

  • 전반적인 테스트 코드 작성이 올바른지 확인해주시면 감사하겠습니다

라이브러리 충돌 에러는 최대한 빠른 시일 내로 해결하도록 하겠습니다.

@Pjhn Pjhn changed the title 부산대 Android 박정훈 3주차 과제 Step2 부산대 Android 박정훈 4주차 과제 Step2 Jul 19, 2024
@LeeOhHyung LeeOhHyung self-requested a review July 21, 2024 16:41
Copy link

@LeeOhHyung LeeOhHyung left a comment

Choose a reason for hiding this comment

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

4주차 과제도 고생많으셨습니다!

Comment on lines +10 to +13

class PlaceRepositoryImpl(context: Context):
SQLiteOpenHelper(context, PlaceContract.DATABASE_NAME, null, 1),
PlaceRepository {

Choose a reason for hiding this comment

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

장소와 관련된 데이터를 담당하는 Repository 이니, Repository가 SQLiteOpenHelper를 상속받기 보다는 생성자를 통해 open helper 객체를 전달받는 것이 조금더 좋을것 같습니다. 내부에서는 local, remote에 따라서 알맞은 객체로 연결을 해주는 형태가 되는거죠.

Copy link
Author

Choose a reason for hiding this comment

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

확인했습니다! 수정하도록 하겠습니다

Comment on lines 116 to 140
private fun initMap() {
val sharedPreferences = getSharedPreferences("LastVisitedPlace", MODE_PRIVATE)
val placeName = sharedPreferences.getString("placeName", null)
val roadAddressName = sharedPreferences.getString("roadAddressName", null)
val categoryName = sharedPreferences.getString("categoryName", null)
val yPos = sharedPreferences.getString("yPos", null)
val xPos = sharedPreferences.getString("xPos", null)

if (placeName != null && roadAddressName != null && categoryName != null && yPos != null && xPos != null) {
val place = Place("", placeName, roadAddressName, categoryName, xPos, yPos)
updateMapWithPlaceData(place)
showBottomSheet(place)
}
}

private fun saveLastVisitedPlace(place: Place) {
val sharedPreferences = getSharedPreferences("LastVisitedPlace", MODE_PRIVATE)
val editor = sharedPreferences.edit()
editor.putString("placeName", place.place)
editor.putString("roadAddressName", place.address)
editor.putString("categoryName", place.category)
editor.putString("yPos", place.yPos)
editor.putString("xPos", place.xPos)
editor.apply()
}

Choose a reason for hiding this comment

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

sharedPreference에 저장하는것도 기기에 데이터를 저장한다는 관점에서는 data layer에서 수행하는게 어떨까 합니다. UI layer에서는 저장된 place 정보만 전달받아 지도에 보여주는 형태이고, 저장된 place를 가져오는 행위는 ViewModel에서 Repository를 통해서 수행하는것이 조금더 적절할것 같습니다

Comment on lines 42 to 56
private val _places = searchText.asFlow()
.debounce(500L)
.flatMapLatest { query ->
if (query.isNotBlank()) {
liveData {
updateTempPlaces(query)
delay(300L)
emit(getAllLiveDataPlaces())
}.asFlow()
} else {
flowOf(emptyList<Place>())
}
}
.asLiveData()
val places: LiveData<List<Place>> get() = _places

Choose a reason for hiding this comment

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

UI로 데이터를 전달해주기 위한 수단으로도 flow를 사용할 수 있으니, LiveData로 변환할 필요는 없을듯합니다. 이참에 flow를 사용해보는것도 도전해보시죠!

Copy link
Author

Choose a reason for hiding this comment

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

넵 flow 사용하는 것으로 코드 수정했습니다!

Comment on lines 96 to 116
private fun updateTempPlaces(keyword: String, page: Int = 1): List<Place>{
val tempPlaces = mutableListOf<Place>()
val MAX_PAGE = 3
val SIZE = 15
for (i in page..MAX_PAGE) {
kakaoApiLauncher.getPlacesBySearchText(
BuildConfig.KAKAO_REST_API_KEY, keyword,SIZE, i,
onSuccess = { places ->
tempPlaces.addAll(places)
if (i == MAX_PAGE) {
updatePlaces(tempPlaces)
tempPlaces.clear()
}
},
onError = { errorMessage ->
Toast.makeText(getApplication(), errorMessage, Toast.LENGTH_SHORT).show()
}
)
}
return tempPlaces
}

Choose a reason for hiding this comment

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

카카오 api를 통해 장소를 조회하는것도 data network layer에서 수행되는게 조금더 좋을것 같습니다.
AAC ViewModel은 UI layer로 볼수있으므로, 여기서는 조회해온 place데이터만 넘겨받는 형태가 어떨까요.

Copy link
Author

Choose a reason for hiding this comment

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

넵 확인했습니다.

Comment on lines 75 to 79
private fun showErrorPage(error: Exception){
setContentView(R.layout.error_page)
tvErrorMessage = findViewById(R.id.tvErrorMessage)
tvErrorMessage.text = "지도 인증에 실패했습니다.\n다시 시도해주세요.\n"+ error.message
}

Choose a reason for hiding this comment

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

에러 페이지를 보여주고 싶을때 Activity의 setContentView 를 다시 호출하기 보다는 XML 위 선언된 View를 visible/gone 처리만 해주는것이 좋을것 같습니다. 지금은 네트워크 연결이 끊어졌을때는 에러페이지가 보이지만, 다시 연결되었을때 정상화면이 보이는처리도 필요해보이네요

Pjhn added 3 commits July 22, 2024 03:24
- 패키지명 변경: view -> presentation
- 파일명 변경
- ViewActivity.kt->  SearchActivity.kt
- PlaceViewModel.kt -> SearchViewModel.kt
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