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_김주송_6주차 과제 (STEP2) #75

Open
wants to merge 120 commits into
base: jooiss
Choose a base branch
from

Conversation

jooiss
Copy link

@jooiss jooiss commented Aug 2, 2024

코드 작성 시, 어려웠던 점

  • 문서가 잘 정리되어 있고 참고자료가 많이 있어 비교적 쉽게 수행하였습니다.
  • 토큰 추가하는 칸이 입력 칸인지 몰라서 조금 헤맸습니다.

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

  • STEP1 코멘트를 보고 ViewModel로 나눠봤는데 이것이 잘 분리되었는지 확인하고 싶습니다.
  • 패키지별로 잘 분리되었는지에 대해 궁금합니다. (Service의 경우는 어떤 패키지에 넣어야 할 지 잘 모르겠어 application처럼 밖으로 빼놨는데 비즈니스 로직을 처리하는 domain layer에 넣는 것이 맞나 하는 의문이 들었습니다.)

실행 화면

foreground_notification

멘토님께

6주간 코드 리뷰 해주셔서 너무 많이 성장하고 도움 받았습니다. 정말 감사합니다!
항상 부족한 코드 리뷰해주셔서 감사하고 마지막까지 열심히 학습하겠습니다 😄

MyStoryG and others added 30 commits June 25, 2024 01:19
- PlaceContract : 테이블 이름 및 열 이름 정의
- PlaceDatabaseHelper : 데이터베이스 생성
- PlaceDatabaseAccess : 데이터베이스에 데이터 저장 및 삭제 기능
- place_item : place list item layout
- search_item : search list item layout
- PlaceDataModel : Place 데이터 클래스
- PlaceRecyclerViewAcitivity : Place 목록을 보여줄 recycler view 제어
- PlaceRecyclerViewAdapter : Place 목록 adapter
- STEP2 구현 기능 목록 추가
@LeeOhHyung LeeOhHyung self-requested a review August 2, 2024 12:03
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.

@jooiss Good 👍 6주차 과제도 고생많으셨습니다. 멘토링때 말씀드렸던 UiModel 위치도 바꿔주셨네요. 코멘트 확인한번 부탁드립니다. PR은 일요일 심야시간 혹은 월요일에 머지하도록 할게요!

Comment on lines 31 to 33
private fun sendRegistrationToServer(token: String?) {
Log.d("notification", "sendRegistrationTokenToServer($token)")
}

Choose a reason for hiding this comment

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

백엔드에서 FCM을 발송해주고 싶을때는 token을 서버로 보내는 과정이 필요하긴 합니다. 현재는 로그만 찍고 있는것 같네요. 추후에 필요해서 미리 만들어두신건가요?

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 175 to 178
if (ContextCompat.checkSelfPermission(this, android.Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED) {
// FCM SDK (and your app) can post notifications.
}
else if (shouldShowRequestPermissionRationale(android.Manifest.permission.POST_NOTIFICATIONS)) {

Choose a reason for hiding this comment

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

!= 으로 권한 허용이 안되어 있는 경우로 확인한다면, if-else 문을 하나 줄일 수 있을것 같습니다.

Comment on lines 27 to 31
else {
splashViewModel.serviceMessage.observe(this) { serviceMessage ->
splashBinding.tvError.text = serviceMessage
}
}

Choose a reason for hiding this comment

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

observer 내에서 다른 LiveData의 observer를 설정해주는건 추천하는 방법이 아닙니다.
observe(this)... 코드를 각각 선언되게 하더라도 serviceMessage를 전달받을 수 있을거에요!

// SplashActivity.kt
splashViewModel.serviceState.observe(this) { serviceState -> 
 ...
}

splashViewModel.serviceMessage.observe(this) { serviceMessage ->
    splashBinding.tvError.text = serviceMessage
}

Comment on lines 23 to 30
remoteConfig.fetchAndActivate().addOnCompleteListener { task ->
if (task.isSuccessful) {
val serviceState = remoteConfig.getString("serviceState")
val serviceMessage = remoteConfig.getString("serviceMessage")
_serviceState.value = serviceState
_serviceMessage.value = serviceMessage
}
}

Choose a reason for hiding this comment

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

SplashActivity에서 serviceState == "ON_SERVICE" 이 조건을 확인하고 있지만, task.isSuccessful 이 아닐때는 LiveData에 값을 넣어주지 않기 때문에 errorMessage를 제대로 보여주지 못하고 있을것 같습니다. 따라서 이곳에서 else문으로 task 실패했을때의 처리도 필요할것 같아요

Comment on lines +12 to +14

class MapFirebaseMessagingService : FirebaseMessagingService() {
override fun onMessageReceived(remoteMessage: RemoteMessage) {

Choose a reason for hiding this comment

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

Service의 경우는 어떤 패키지에 넣어야 할 지 잘 모르겠어 application처럼 밖으로 빼놨는데 비즈니스 로직을 처리하는 domain layer에 넣는 것이 맞나 하는 의문이 들었습니다

Service 컴포넌트는 ui 패키지와는 별도의 공간에 두는편입니다. (물론 정답은 없습니다.)
아래 3가지 정도가 제가 본 일반적인 방법들인것 같습니다.

  • Application 클래스와 동일하게 두는 방법
  • service를 모아두는 패키지를 만드는 방법
  • fcm 관련 클래스를 하나의 패키지로 만드는 방법

@jooiss
Copy link
Author

jooiss commented Aug 4, 2024

멘토님 마지막까지 정성스러운 코멘트 감사합니다. 멘토링 때 말씀해주신 내용 반영해서 메시지 수신 부분과 알림 부분을 분리하고 싶습니다. 조금 더 학습하고 제대로 분리하고 싶어 지금 열심히 하는 중이지만 시간이 꽤나 걸릴 것 같아요. 혹시 가능하시다면 월요일에 머지해주시면 정말 감사하겠습니다!😀

@LeeOhHyung
Copy link

@jooiss 주말에도 바로 커밋해주셨네요. 마지막 2개 커밋 확인했습니다!

@jooiss
Copy link
Author

jooiss commented Aug 5, 2024

수정 사항

  1. Remote config 호출 방식
  • Application 프로세스가 호출될 때 한번 부르고 다른 Activity에서 가져다 쓰는 방식으로 변경하였습니다.
  1. FCM (message 수신 / 알림 부분 분리)
  • 알림 부분을 함수로 분리하였습니다.
  • ⭐️ 이 부분에서 조금 헷갈렸던 것이 메시지 수신했을 때 onMessageReceived()에서 notification을 요청하는 것이 맞는지 헷갈렸습니다. 멘토님께서 말씀하신 분리하라는 말이 제가 한 정도를 말한 것이 맞는지 확인 받고싶습니다!

질문 사항

  1. Channel 분리
  • notification을 보낼 때 channel의 존재여부를 확인한 후, 존재하지 않으면 생성하는 방식으로 사용하라고 말씀해주셨는데
    1️⃣ 새로운 채널을 생성할 때 channel_id, channel_name, channel_inportance, notification_id를 파라미터로 넣어 새로운 채널을 생성하는 방식으로 사용하는지,
    2️⃣ 미리 지정해놓은 스트링을 상황에 맞게 여러개의 채널 생성하는 함수를 만들어놓고 (ex: createDefaultChannel, createTalkChannel) 사용하는지 궁금합니다.
  1. 추가로 제가 한 수정한 부분 중에 위치가 어색하거나 LiveData, 의존성 주입 등 추가하면 좋을 부분에 대해 더 알고 싶습니다.

마지막까지 도움주셔서 감사합니다! 좋은 하루 되세요 🙂

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.

3 participants