부산대 Android 박정훈 5주차 과제 Step2 #85
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- local, remote DB 객체 연결
- PlaceDBHelper: 로컬 데이터베이스에서 장소 정보를 가져오고 저장하는 기능 유지
- 에러 텍스트를 네트워크 상태에 따라 visible/gone으로 처리하는 것으로 변경
- 지도 화면을 스와이프 하면 네트워크 상태에 따라 화면 갱신
omjoonkim
pushed a commit
that referenced
this pull request
Jul 28, 2024
* Initial commit * 충남대 Android_김선규 4주차 Step0 (#10) * Initial commit * Merge : android-map-keyword into android-map-search (#8) * 충남대 Android_김선규 3주차 과제 Step1 (#47) * docs: add step1 requirements * chore: set for using android api * style: rename id in layout * feat: remove storeInfo for using api * feat: add connecting api for searching * style: rename variable name proper * 충남대 Android_김선규 3주차 과제 Step2 (#85) * style: function rename and split * feat: Change function to fit coroutine * docs: add step2 requirements * style: move from main to sub file * chore: set it up to work in the right environment * feat: display kakao map, when app is started --------- Co-authored-by: MyStoryG <[email protected]> * 충남대 Android_김선규 4주차 Step 1 제출 (#47) * docs: add week 4 step 1 requirements * feat: add searching by saved search keyword * chore: relocate files proper * feat: modify adapter to make clean code * feat: add image for marker * feat: add layout for displaying bottom sheet * feat: add parcelabel for easy to send data * feat: add displaying search result * refactor: modify class structure * feat: add error screen and reload button * feat: add saving and loading last position when app is closed, save last position when app is opened, load last position * 충남대 Android_김선규 4주차 Step2 수정 (#73) * style: rename variable name * feat: add viewModel and Repository for saving last position * refactor: classify in more detail * test: add android UI test * chore: add mockk test dependency * test: add ViewModel test * chore: add testOptions * test: modify android ui test --------- Co-authored-by: MyStoryG <[email protected]>
- constraintlayout 삭제
Pjhn
force-pushed
the
step2
branch
3 times, most recently
from
92e2e28 to
e4c2b83
Compare
kimseongyu
added a commit
to kimseongyu/android-map-refactoring
that referenced
this pull request
Jul 29, 2024
* Initial commit * Merge : android-map-keyword into android-map-search (kakao-tech-campus-2nd-step2#8) * 충남대 Android_김선규 3주차 과제 Step1 (kakao-tech-campus-2nd-step2#47) * docs: add step1 requirements * chore: set for using android api * style: rename id in layout * feat: remove storeInfo for using api * feat: add connecting api for searching * style: rename variable name proper * 충남대 Android_김선규 3주차 과제 Step2 (kakao-tech-campus-2nd-step2#85) * style: function rename and split * feat: Change function to fit coroutine * docs: add step2 requirements * style: move from main to sub file * chore: set it up to work in the right environment * feat: display kakao map, when app is started --------- Co-authored-by: MyStoryG <[email protected]>
LeeOhHyung
approved these changes
Jul 30, 2024
There was a problem hiding this comment.
@Pjhn LGTM~ 5주차 과제 고생많으셨습니다. 점점 발전하는 모습이네요
local, remote 레포지토리를 상속 관계로 구현하였습니다. 또한, remote 레포지토리에서 API 호출 작업을 하도록 코드를 수정했는데, 이러한 방식이 적절한지 궁금합니다
큰 문제점을 발견하지는 못했습니다. ViewModel 에서 repository의 메소드를 호출하고, repository내에서 local/remote에 맞춰서 적절한 메소드를 또 호출하게 하는게 일반적으로 작성하는 형태는 맞습니다.
나머지 패키지 분리 및 의존성 주입부분도 빠르게 적용해주신것 같네요.
남긴 코멘트는 6주차 과제에 반영해주셔도 괜찮습니다.
Comment on lines
+27
to
+28
| @Volatile | ||
| private lateinit var appInstance: PlaceApplication |
There was a problem hiding this comment.
appInstance 를 활용하고자 하신곳이 있을까요? 이 과제에서는 없는듯 하네요.
만약에 application context 를 사용하고 싶으신것이라면, hilt module에 @ApplicationContext 어노테이션을 사용해서 가져올 수 있는 방법이 있습니다. (참고 블로그)
|
|
||
| class LastVisitedPlaceManager @Inject constructor(context: Context) { | ||
|
|
||
| private val sharedPreferences = context.getSharedPreferences("LastVisitedPlace", Context.MODE_PRIVATE) |
There was a problem hiding this comment.
생성자로 context를 주입받아서 클래스 내부에서 선언해도 되겠지만, shared preference 자체를 hilt module에 선언하고, 생성자로 전달받도록 할 수 있을것 같습니다
Comment on lines
+20
to
+27
| for (page in 1..3) { | ||
| val response = kakaoApi.getSearchKeyword( | ||
| key = BuildConfig.KAKAO_REST_API_KEY, | ||
| query = placeName, | ||
| size = 15, | ||
| page = page | ||
| ) | ||
| if (response.isSuccessful) { |
There was a problem hiding this comment.
아마도 페이지네이션 이나 한번에 3개의 페이지를 불러오고 싶은 상황인것 같습니다. 이럴때는 내부에서 loop를 도는것 보다는 getPlaces() 메소드 자체가 3번 호출되는 방향이 좀더 적절하지 않을까 싶습니다
Comment on lines
+5
to
+7
| data class ResultSearchKeyword( | ||
| var documents: List<Place> | ||
| ) |
There was a problem hiding this comment.
data class의 멤버변수는 val 로 선언되는게 적절해보이네요!
Comment on lines
+27
to
+28
| private lateinit var searchedPlaceAdapter: SearchedPlaceAdapter | ||
| private lateinit var logAdapter: LogAdapter |
There was a problem hiding this comment.
필요한 adapter를 lateinit var 클래스로 만들어도 되지만, 앱 실행동안에 인스턴스 자체를 새롭게 할당해주는 경우는 없을겁니다. 따라서 by lazy를 통해 지연초기화 하는 방법도 있으니 참고하시면 좋을것 같습니다. (참고 블로그)
Comment on lines
+34
to
+38
| init { | ||
| viewModelScope.launch(Dispatchers.IO) { | ||
| _logList.value = getLogs() | ||
| } | ||
| } |
There was a problem hiding this comment.
setValue로 라이브 데이터에 값을 넣어주고 있기 때문에 Dispatchers.IO 를 넣어주지 않아도 괜찮을것 같습니다
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
코드 작성하면서 어려웠던 점
hilt를 사용한 의존성 적용하는 부분이 생소하였습니다.
코드 리뷰 시, 멘토님이 중점적으로 리뷰해줬으면 하는 부분
(local, remote 레포지토리를 상속 관계로 구현하였습니다. 또한, remote 레포지토리에서 API 호출 작업을 하도록 코드를 수정했는데, 이러한 방식이 적절한지 궁금합니다)