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

#295 [feat] 푸시알림 구현 #305

Merged
merged 7 commits into from
Mar 11, 2024
Merged

#295 [feat] 푸시알림 구현 #305

merged 7 commits into from
Mar 11, 2024

Conversation

2zerozu
Copy link
Contributor

@2zerozu 2zerozu commented Mar 3, 2024

관련 이슈

작업 사진/동영상 (선택)

작업한 내용

PR 포인트

Copy link
Member

@stellar-halo stellar-halo left a comment

Choose a reason for hiding this comment

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

너무 늦은 코리 미안합니다.. 오랜만에 보는 영주 코드 너무 잘 읽혀서 좋았어요. 😻

override fun getFcmToken(setFcmToken: (String) -> Unit) {
FirebaseMessaging.getInstance().token.addOnCompleteListener(
OnCompleteListener { task ->
if (!task.isSuccessful) {
Copy link
Member

Choose a reason for hiding this comment

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

task.isSuccessful.not() 라는 것도 있긴 한데,, 어떤 것이 더 나을진 모르겠습니다

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

task.isSuccessful.not() 라는 것도 있긴 한데,, 어떤 것이 더 나을진 모르겠습니다

흐으음 뭔가 not successful이 더 잘 어울리는 거 같아서 맨뒤에 not 키워드를 붙이는 건 제 개인적인 의견으론 가독성이 떨어져보이네요 .. 이건 우리끼리 컨벤션을 맞춰보는 게 좋을 거 같아요 ~~

Copy link
Member

Choose a reason for hiding this comment

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

저도 동의하는 바입니다.. 마지막에 not은 별론데 !이거는 제 눈에는 잘 안들어와서 또 별로더라고요,,컨벤션 함 이야기 해보아요!

@@ -33,4 +36,16 @@ class AuthRepositoryImpl @Inject constructor(
override fun setSignedUp() {
localSignedUpDataSource.isSignedUp = true
}

override fun getFcmToken(setFcmToken: (String) -> Unit) {
Copy link
Member

Choose a reason for hiding this comment

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

inline으로 쓰는 건 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inline으로 쓰는 건 어떤가요?

이유를 알 수 있을까요?!

Copy link
Member

Choose a reason for hiding this comment

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

람다 파라미터를 받아올 때 더 이롭다고 알고 있습니다! https://jaehochoe.medium.com/better-kotlin-inline-2c41f1db0dfc

Copy link
Contributor Author

@2zerozu 2zerozu Mar 8, 2024

Choose a reason for hiding this comment

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

람다 파라미터를 받아올 때 더 이롭다고 알고 있습니다! https://jaehochoe.medium.com/better-kotlin-inline-2c41f1db0dfc

inline을 쓰면 람다를 표현하는 클래스와 람다 인스턴스에 해당하는 객체를 만들 필요도 없어서 이롭긴 하지만 람다를 사용하는 모든 함수에 inline를 사용할 순 없습니다

inline은 컴파일 시점에서 함수 호출 부분에 함수의 본문 코드가 삽입되는 방식으로 동작하는데, 현재 코드같은 경우는 인터페이스를 상속받아 구현이 런타임에서 결정되기 때문에 inline이 작동하지 않을 수도 있어요

실제로 저 함수에 inline 키워드를 붙여 디컴파일 해보면 (위에가 기존 코드, 아래가 inline 붙인 코드)
image
코드가 별반 다를 바가 없음을 알 수 있습니다 (final 붙은 정도 ..?)
그래서 현재 코드에 inline을 넣는다고 크게 효과보는 건 없다고 생각이 드네요

Copy link
Member

Choose a reason for hiding this comment

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

오호라 상세한 답변 감사합니다

Copy link
Contributor

@hajeong67 hajeong67 left a comment

Choose a reason for hiding this comment

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

어푸어푸 늦어서 죄송합니다!

@2zerozu 2zerozu added this to the 1차 스프린트 milestone Mar 8, 2024
@2zerozu 2zerozu self-assigned this Mar 8, 2024
@2zerozu 2zerozu added feat 새로운 기능 추가 Pull Request🔥 풀리퀘 날림! 영주🐼 영주가 작업함! labels Mar 8, 2024
@2zerozu 2zerozu merged commit ce4af6e into develop Mar 11, 2024
1 check passed
@2zerozu 2zerozu deleted the feature/#295-firebase branch March 11, 2024 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 새로운 기능 추가 Pull Request🔥 풀리퀘 날림! 영주🐼 영주가 작업함!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] 푸시알림 구현
3 participants