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 #56

Open
wants to merge 8 commits into
base: ichanguk
Choose a base branch
from

Conversation

ichanguk
Copy link

@ichanguk ichanguk commented Aug 1, 2024

Step2 커밋 모음 링크

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

  • Firebase 사이트 내 문서로만 구현을 하고 싶었지만 좀 어려워서 카테캠 강의 자료를 많이 참고했습니다.

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

  • 전체적으로 한 번씩 봐주시면 좋을 것 같습니다.(README.md도 잘 구성됐는지 한 번 봐주시면 좋을 것 같습니다.)

실행 화면

앱 실행 중 알림               백그라운드 알림

week6_noti_fore week6_noti_back

Copy link

@bigstark bigstark left a comment

Choose a reason for hiding this comment

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

창욱님 6주동안 정말 고생 많으셨어요. 1주차에 비해서 정말 많은 성장을 한 것 같고, 그에 조금은 기여했다고 생각하니 조금은 뿌듯하네요 ㅎㅎ

코드 리뷰는 이번이 마지막이 되겠지만, 현업에서나 행사에서나 뵙게될 날을 기대하고 있겠습니다! ㅎㅎ

Comment on lines +22 to +23
minimumFetchIntervalInSeconds = 0 // 개발용
// minimumFetchIntervalInSeconds = 3600 // 배포용(1시간)
Copy link

Choose a reason for hiding this comment

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

이런 부분들은 flavor 를 통해서 관리되면 좋을 것 같아요 :)

환경에 따라 dev / stage / prod 등으로 나뉘기도 하고, build type 에 따라 debug / release 로도 나뉠 수 있으니, 직접적으로 설정하는 것보다는 flavor 를 통해서 처리되면 좋습니다!

private val TAG = "FirebaseService"

override fun onNewToken(token: String) {
Log.d(TAG, "Refreshed token: $token")
Copy link

Choose a reason for hiding this comment

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

onNewToken 은 갱신된 토큰이 내려오는 편이에요. 보통 data store 등에 저장해두었다가 서버에 보내는 작업을 진행하기도 합니다.


private fun sendNotification(remoteMessage: RemoteMessage) {
notificationManager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager
createNotificationChannel()
Copy link

Choose a reason for hiding this comment

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

notification channel 은 매번 처리하기보다, 한번만 처리하는 편이 좋을 것 같네요!

Comment on lines +75 to +79
fun getFirebaseToken() {
FirebaseMessaging.getInstance().token.addOnSuccessListener {
Log.d(TAG, "token=$it")
}
}
Copy link

Choose a reason for hiding this comment

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

사용하지 않는 코드인것 같네요!

Comment on lines +34 to +36
const val SERVICE_STATE = "serviceState"
const val ON_SERVICE = "ON_SERVICE"
const val SERVICE_MESSAGE = "serviceMessage"
Copy link

Choose a reason for hiding this comment

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

SERVICE_STATE 와 SERVICE_MESSAGE 는 외부 참조할 필요가 없기 때문에 private 처리해주시는 것이 좋을 것 같아요.

Comment on lines +12 to +14
class DefaultFirebaseRemoteConfigRepository @Inject constructor(
private val firebaseRemoteConfig: FirebaseRemoteConfig,
) : FirebaseRemoteConfigRepository {
Copy link

Choose a reason for hiding this comment

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

외부에 구현체는 드러내지 않기 위해서 internal 처리를 해주시는 것이 좋습니다.


companion object ConfigKeys {
const val SERVICE_STATE = "serviceState"
const val ON_SERVICE = "ON_SERVICE"
Copy link

Choose a reason for hiding this comment

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

ON_SERVICE 의 의도가 외부에서 참조하는 것이라면, 구현체에 존재하기 보다는 interface 에 존재하는 것이 좋을 것 같네요.

Comment on lines +75 to +81
private fun handleRemoteConfig(serviceState: String?, serviceMessage: String?) {
if (serviceState == DefaultFirebaseRemoteConfigRepository.ON_SERVICE) {
navigateToMapActivity()
} else {
binding.splashMessageTextView.text = serviceMessage
}
}
Copy link

Choose a reason for hiding this comment

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

음 이부분 로직을 SplashViewModel 로 옮기는 거싱 어때요? 굳이 remote config 에 대한 것을 view 가 알 필요도 없고요.

message 를 표시하거나, map activity 로 navigate 하거나의 상태값만 아는 것이 바람직할 것 같네요!

Comment on lines +23 to +24
private val _errorMessage = MutableStateFlow<String?>(null)
val errorMessage: StateFlow<String?> get() = _errorMessage
Copy link

Choose a reason for hiding this comment

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

UiState 를 따로 정의하고 그 안에 error message 를 숨기는 것이 좋을 것 같아요.
만약 그렇게 하지 않더라도 nullable 처리를 하는 것은 큰 이점이 존재하지 않아요. empty string 을 default 로 처리해주시면 좋을 것 같네요!

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