[부나] 1, 2단계 영화 극장 선택 제출합니다. #15

tmdgh1592 · 2023-04-27T08:31:17Z

안녕하새요 남잭슨!
첫 리뷰어셨는데 다시 만나서 너무 반갑습니다~!
이번 미션 리뷰 잘 부탁드립니다!! 링크

- Reminder is related with notification

namjackson

안녕하세요 부나!
이번 미션도 잘부탁해요!
1~2단계 미션 고생하셨어요,
몇가지 코멘트 남겼으니, 확인해주세요!

namjackson · 2023-04-28T08:13:48Z

app/src/main/java/woowacourse/movie/presentation/activities/main/MainActivity.kt

+                    supportFragmentManager.commit {
+                        replace(R.id.fragment_container_view, HistoryFragment.newInstance())
+                    }
+                }
+
+                R.id.home -> {
+                    supportFragmentManager.commit {
+                        replace(R.id.fragment_container_view, HomeFragment.newInstance())
+                    }
+                }
+
+                R.id.setting -> {
+                    supportFragmentManager.commit {
+                        replace(R.id.fragment_container_view, SettingFragment.newInstance())
+                    }


중복 코드는 함수로 분리해 보아요!

별도의 함수로 분리해서 가독성과 재사용성을 높여보겠습니다!

namjackson · 2023-04-28T08:14:52Z

app/src/main/java/woowacourse/movie/presentation/activities/main/MainActivity.kt

+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
+            if (shouldShowRequestPermissionRationale(Manifest.permission.POST_NOTIFICATIONS)) {
+                requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
+            } else {
+                requestPermissionLauncher.launch(Manifest.permission.POST_NOTIFICATIONS)
+            }
+        }


뎁스를 더 줄여봐도 좋을거같아요!
if-else문이 동일하네요 왜일까요?

이 부분은 나중에 작업하려고 남겨두고 수정하지 않은 것 같습니다 😅

앱을 처음 실행했을 때 shouldShowRequestPermissionRationale() 반환이 참일 때 권한을 요구한다면, API 30 미만에서는 권한을 거부해도 앱을 켤 때마다 계속 권한 요청을 호출할 것입니다.
그렇기 때문에 false일 때만 권한 요청을 하도록 수정하겠습니다!

namjackson · 2023-04-28T08:15:58Z

app/src/main/java/woowacourse/movie/presentation/activities/main/alarm/PushAlarmManager.kt

+    private val pendingIntent = intent.let { intent ->
+        intent.action = PUSH_ACTION
+        intent.putExtra(PUSH_DATA_KEY, data)
+        PendingIntent.getBroadcast(context, 0, intent, PendingIntent.FLAG_IMMUTABLE)


0은 무엇을 의미할까요?
두번째 받은 푸시는 내예약화면으로 잘보내줄까요?

찾아보니 두 번째 인자는 RequestCode로, PendingIntent를 가져오기 위한 고유 식별자 역할을 한다고 합니다.
지금같은 상황에서는 0으로 고정되어 있기 때문에 2번 예약을 해도 정상적으로 동작하지 않을 것으로 기대됩니다!

아래와 같은 코드로 고유한 값으로 설정하도록 변경하였습니다.
private fun getUniqueNumber(): Int = System.currentTimeMillis().toInt()

namjackson · 2023-04-28T08:16:43Z

app/src/main/java/woowacourse/movie/presentation/activities/main/alarm/PushAlarmManager.kt

+    data: T,
+) {
+    private val alarmManager = context.getSystemService(Context.ALARM_SERVICE) as AlarmManager
+    private val pendingIntent = intent.let { intent ->


let을 사용한 이유는 무엇일까요? 불필요해보여요!

intent를 사용하여 PendingIntent를 생성하고자 위와 같이 작성하였습니다.
하지만 null 체크를 해줄 것이 아니기 때문에 아래와 같이 코드를 수정하였습니다!

private val pendingIntent = run { intent.action = PUSH_ACTION intent.putExtra(PUSH_DATA_KEY, data) PendingIntent.getBroadcast(context, getUniqueNumber(), intent, PendingIntent.FLAG_IMMUTABLE) }

namjackson · 2023-04-28T08:19:24Z

...ain/java/woowacourse/movie/presentation/activities/main/fragments/history/HistoryFragment.kt

+            startActivity(
+                Intent(requireContext(), TicketingResultActivity::class.java)
+                    .putExtra(TicketingResultActivity.RESERVATION_KEY, item)
+            )


클릭이벤트시 동작을 함수로 분리해보아요!
intent를 생성하는 방법을 통일해서 일관성있도록 수정해보아요!

intent를 생성하는 방법을 통일하는 것을 어떤 것을 의미하시는 건가요?!
각 Activity의 companionObject로 intent() 팩토리 함수를 만드는 것도 이에 해당하는지 궁금합니다!

namjackson · 2023-04-28T08:21:43Z

...ain/java/woowacourse/movie/presentation/activities/main/fragments/history/HistoryFragment.kt

+        fun newInstance(): HistoryFragment = historyFragment.apply {
+            arguments = Bundle().apply {}
+        }


불필요해 보이는 코드가 있어요!

namjackson · 2023-04-28T08:27:02Z

app/src/main/java/woowacourse/movie/presentation/activities/ticketing/SeatPickerActivity.kt

    private fun startTicketingResultActivity() {
+        val reservation = Reservation(
+            movieTitle = movie.title,
+            movieDate = movieDate.toPresentation(),
+            movieTime = movieTime.toPresentation(),
+            ticket = ticket,
+            seats = pickedSeats.toPresentation(),
+            ticketPrice = calculateTotalPrice()
+        )
+
+        registerPushBroadcast(reservation)


startTicketingResultActivity라는 이름이지만, 다른 동작도 함께하고있네요!

Reservation을 따로 생성해주진 않네요!,,
Reservation을 생성해주는것도 도메인이 해야할 책임아닐까요?

예약 알림 Broadcast를 보내는 함수를 따로 호출해주도록 변경하였습니다!

…구현하도록 수정

- Application 의 생명주기 작동하지 않는 경우가 있기 때문

tmdgh1592 · 2023-04-28T17:52:47Z

안녕하세요 잭슨!
코드를 수정하여 다시 리뷰요청을 보냅니다 : )

페어의 리뷰를 함께 참고하여 수정하던 도중 궁금한 점이 생겨 질문을 드리고 싶습니다.
기존에는 Application에서 Preference 객체를 생성해주었는데, Application의 생명주기가 작동하지 않는 곳에서는 문제가 생길 수 있다는 글을 보았습니다.
앱을 실행하면 Application 생명주기가 무조건 실행되는 것으로 알고 있는데 이런 문제가 발생할 수 있는 상황이 어떤게 있는지 궁금합니다!
구글링을 해보았지만 적절한 답변을 얻지 못하여 질문 드립니다..!

…티 타입을 전달하도록 변경

namjackson

안녕하세요 부나!
피드백 적용 잘해주셨네요! 💯
몇가지 코멘트 남겼으니, 확인해주세요 😄
코멘트는 다음미션 진행하면서 참고해주세요 👍

namjackson · 2023-04-29T15:43:42Z

app/src/main/java/woowacourse/movie/presentation/activities/ticketing/SeatPickerActivity.kt

+    private fun makeReservation(): Reservation = DomainReservation.of(
+        movieTitle = movie.title,
+        year = movieDate.year,
+        month = movieDate.month,
+        day = movieDate.day,
+        hour = movieTime.hour,
+        min = movieTime.min,
+        ticketCount = ticket.count,
+        seats = pickedSeats,
+        price = calculateTotalPrice(),
+    ).toPresentation()


Reservation라는 도메인 객체를 생성해주셨지만,
여전히 객체의 생성은 View로직과 섞여있는거 같아요!

"영화를 예약한다" 라는것 또한 도메인 로직이 아닐까라는 생각이 들어요!
이부분은 추가로 고민해보셔도 좋을거같아요!

namjackson · 2023-04-29T15:44:34Z

...ain/java/woowacourse/movie/presentation/activities/main/fragments/history/HistoryFragment.kt

-            Intent(requireContext(), TicketingResultActivity::class.java)
-                .putExtra(TicketingResultActivity.RESERVATION_KEY, item)
-        )
+        startActivity(TicketingResultActivity.intent(requireContext(), item))


함수명은 동사나 동사구로 지으면 어덜까요?

namjackson · 2023-04-29T16:08:42Z

data/src/main/java/com/woowacourse/data/local/LocalDataStore.kt

+        private var instance: LocalDataStore? = null
+        private lateinit var preference: SharedPreferences


LocalDataStore instance와 preference 를 따로 저장한 이유가 있나요?

LocalDataStore 내에서 preference를 사용하고 있어서 Preference 또한 한 번만 생성하고자 따로 저장하였습니다.

namjackson · 2023-04-29T16:10:33Z

app/src/main/java/woowacourse/movie/presentation/reminder/Reminder.kt

+import woowacourse.movie.R
+import woowacourse.movie.presentation.extensions.checkPermissionTiramisu
+
+abstract class Reminder {


Reminder라는 기능보다는,
Notification관련 클래스는 아닐까요?

네이밍을 고민해보아요!

다시 보니 Reminder라기엔 단순 알람을 보내는 기능만 수행하고 있네요..!
직관저긍로 푸시 알람을 보내는 역할을 한다고 알 수 있도록 변경해보겠습니다!

namjackson · 2023-04-29T16:12:05Z

app/src/main/java/woowacourse/movie/presentation/reminder/Reminder.kt

+            .setContentIntent(onClickNotification())
+
+        if (isPermissionGranted(context)) {
+            NotificationManagerCompat.from(context).notify(pushId, builder.build())


pushId는 무엇을 의미할까요?
동시에 여러 노티가 오게된다면 어떻게 될까요?

기존 노티는 제거되고 마지막 노티만 남게 되는 문제가 있습니다.
이를 사용하는 곳에서 고정값 1000을 사용하고 있었는데, 대신에 data의 hashcode를 전달하도록 변경하였습니다.
다른 data인 경우 새로운 알림이 전송되고, 동일한 데이터에 대한 노티는 리프레시되도록 하였습니다.

namjackson · 2023-04-29T16:57:20Z

data/src/main/java/com/woowacourse/data/DataStore.kt

@@ -0,0 +1,6 @@
+package com.woowacourse.data
+
+interface DataStore {


페어의 리뷰를 함께 참고하여 수정하던 도중 궁금한 점이 생겨 질문을 드리고 싶습니다.
기존에는 Application에서 Preference 객체를 생성해주었는데, Application의 생명주기가 작동하지 않는 곳에서는 문제가 생길 수 있다는 글을 보았습니다.
앱을 실행하면 Application 생명주기가 무조건 실행되는 것으로 알고 있는데 이런 문제가 발생할 수 있는 상황이 어떤게 있는지 궁금합니다!
구글링을 해보았지만 적절한 답변을 얻지 못하여 질문 드립니다..!

앱이 실행되다면, Application이 실행되긴하지만,
Application 또한 여러 이유로 OS내에서 제거되거나, 다시 생성될 여지가 있긴해요!
물론 일반적으로 동작에는 큰 지장은 없을거지만,
Application에서 객체를 생성해주게되면, 메모리 측면이나 성능측면에서 문제가 되기도해요.
또, Application으로부터 객체를 가져오는 동작도, 어느서나 객체를 가져올수 있어 편하지만,
사용하는 클래스에서는 Application에 대한 의존이 생기게 됩니다!
이런 의존성은 관리하기도 힘들고 테스트도 힘든구조가 되곤합니다!
아마 이런 부분들을 말씀하신거같아요! :)

tmdgh1592 added 28 commits April 25, 2023 15:04

feat: 영화 티켓 예매 마이그레이션

0e6a17d

docs: 예매 내역 기능 구현 목록 작성

fe3b115

feat(MainActivity): 바텀 내비게이션 추가

8699f04

feat(MainActivity): 바텀 네비게이션 클릭 시 해당 프래그먼트로 이동하는 기능 구현

41e7187

feat(HistoryFragment): 리사이클러뷰 예매 내역 아이템 xml 구현

85207ff

refactor(Reservation): 예약 정보를 Reservation 모델로 전달하도록 변경

0f15d0b

feat(HistoryFragment): 예매 내역 목록을 보여주는 기능 구현

cb38495

feat(HistoryFragment): 예매 내역 목록을 보여주는 기능 구현

b6ff7d2

feat(MainActivity): 알림권한 허용을 요청하는 기능 구현

08fc1a2

feat(SettingFragment): 설정 화면 구현

1741ebc

feat(Preferences): 설정 데이터를 저장하는 SharedPreference 구현

871d0ed

feat(SettingFragment): 설정에서 알림 기능을 On/Off 하는 기능 구현

2b75a9b

feat(HistoryFragment): 예매 기록 구분선 추가

1426e31

feat: 영화 시작 시간 30분 전에 푸시 알림, 푸시 알림을 클릭하면 예매 정보를 보여주는 기능 구현

74a824e

refactor: AlarmReceiver 클래스명 변경

e1d4aa5

refactor: Reminder를 추상화하도록 변경

a105c5b

- Reminder is related with notification

refactor(PushReceiver): 푸시 알림 수신 여부에 따라 알림 제어

4d54d12

refactor(PushReceiver): 푸시 알림 수신 여부에 따라 알림 제어

c3555ce

refactor(PushAlarmManager): AlarmManager 클래스 분리

4bab86b

refactor(Notification): 아이콘 변경

2a32d40

refactor(PushAlarmManager): set함수에서 시간을 Long타입으로 변환하도록 수정

bffd245

fix(ReservationPushReceiver): 알림이 오지 않던 문제 키 값 변경으로 해결

5fd4448

refactor(MovieApplication): sharedPreferences를 application이 관리하도록 변경

a45577b

충돌해결

dd5e718

refactor(ContextExt): 권한확인 기능을 확장 함수로 분리

f37813b

feat: 권한이 허용되어 있지 않을 때 푸시 스위치를 켜면, 권한 요청 다이얼로그를 보여주는 기능 구현

b0feac7

feat(SettingFragment): 권한이 없는 상태에서 푸시 스위치를 켜면 시스템 설정 화면으로 이동하는 기능 구현

e2cf7fe

test(SettingFragment): 세팅 화면 테스트 코드 작성

491371e

namjackson requested changes Apr 28, 2023

View reviewed changes

refactor: Fragment 이동하는 기능을 함수로 분리

03aa72e

tmdgh1592 added 10 commits April 29, 2023 01:05

refactor: 사용자에게 권한 요청을 한 번만 하도록 변경

d097fac

fix(PushAlarmManager): PendingIntent 생성시 고유한 값을 사용하도록 변경

e491a8f

refactor(PushAlarmManager): pendingIntent 생성을 run 스코프 함수로 초기화하도록 변경

c175522

refactor: 예매 내역 아이템을 클릭했을 때 이벤트를 함수로 분리

b9eac7a

refactor: Fragment의 newInstance() 호출시 불필요한 함수 호출 제거

bb39788

refactor(AlarmManager): set() 함수 호출시 intent와 푸시할 데이터를 전달받도록 변경

d6aae69

refactor(DataStore): Preference의 이름을 LocalDataStore로 변경하고 DataStore를 …

4d37b79

…구현하도록 수정

refactor(LocalDataStore): Application을 제거하고 싱글톤으로 생성하도록 변경

b4189f7

- Application 의 생명주기 작동하지 않는 경우가 있기 때문

refactor(TicketingResultActivity): intent 생성 방식을 동반 객체에서 정의하도록 변경

0c3205f

refactor: Reservation 도메인 생성

f633363

tmdgh1592 added 2 commits April 29, 2023 02:55

fix: 푸시 알림을 30일 이전이 아닌 30분 전으로 수정

1694f40

fix(TicketingResultActivity): intent() 메서드에 companion object가 아니라 액티비…

6eacceb

…티 타입을 전달하도록 변경

namjackson approved these changes Apr 29, 2023

View reviewed changes

namjackson merged commit e2c45bf into woowacourse:tmdgh1592 Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[부나] 1, 2단계 영화 극장 선택 제출합니다. #15

[부나] 1, 2단계 영화 극장 선택 제출합니다. #15

tmdgh1592 commented Apr 27, 2023 •

edited

Loading

namjackson left a comment

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

namjackson Apr 28, 2023

namjackson Apr 28, 2023

namjackson Apr 28, 2023

tmdgh1592 Apr 28, 2023

tmdgh1592 commented Apr 28, 2023

namjackson left a comment

namjackson Apr 29, 2023

namjackson Apr 29, 2023

namjackson Apr 29, 2023

tmdgh1592 Apr 30, 2023

namjackson Apr 29, 2023

tmdgh1592 Apr 30, 2023

namjackson Apr 29, 2023

tmdgh1592 Apr 30, 2023

namjackson Apr 29, 2023

		private var instance: LocalDataStore? = null
		private lateinit var preference: SharedPreferences

		@@ -0,0 +1,6 @@
		package com.woowacourse.data

		interface DataStore {

[부나] 1, 2단계 영화 극장 선택 제출합니다. #15

[부나] 1, 2단계 영화 극장 선택 제출합니다. #15

Conversation

tmdgh1592 commented Apr 27, 2023 • edited Loading

namjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 commented Apr 28, 2023

namjackson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmdgh1592 commented Apr 27, 2023 •

edited

Loading