[스캇] 1, 2단계 영화 극장 선택 제출합니다. #23
Merged
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
laco-dev
requested changes
Apr 27, 2023
app/src/androidTest/java/woowacourse/movie/SettingFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/woowacourse/movie/SettingFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/woowacourse/movie/SettingFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/woowacourse/movie/SettingFragmentTest.kt
Outdated
Show resolved
Hide resolved
Comment on lines
34
to
35
| sharedPreference = | ||
| parentContext.getSharedPreferences(SETTING, AppCompatActivity.MODE_PRIVATE) |
There was a problem hiding this comment.
프리퍼런스에 접근하고 관리하는 코드도 별도 객체로 분리 해 봐도 좋습니다
싱글톤?
There was a problem hiding this comment.
SettingPreferenceManager 를 만들어서 프리퍼런스를 관리하도록 만들었습니다
app/src/main/java/woowacourse/movie/receiver/NotificationHelper.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/woowacourse/movie/receiver/NotificationHelper.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/woowacourse/movie/receiver/ReservationNotificationReceiver.kt
Outdated
Show resolved
Hide resolved
|
안녕하세요! |
laco-dev
approved these changes
Apr 30, 2023
Comment on lines
+24
to
+26
| fragmentScenario?.onFragment { | ||
| isAlarmOn = | ||
| it.requireActivity().findViewById<Switch>(R.id.setting_push_alarm_switch).isChecked |
| } | ||
|
|
||
| private fun getTimeInMillis(localDateTime: LocalDateTime): Long { | ||
| val sqlDate = getAlarmDateTime(localDateTime, ALARM_TIME) |
Comment on lines
+39
to
+41
| private fun generateAlarmManager(context: Context): AlarmManager { | ||
| return context.getSystemService(ALARM_SERVICE) as AlarmManager | ||
| } |
There was a problem hiding this comment.
알람 매니저의 모든 클래스가 정적 함수처럼 동작을 하고 있어요.
상태를 가지고 있지 않다는 의미로 볼 수 있는데요.
이런 경우는 object로 만들어주거나 아니면 클래스로 사용할 수 있도록 만들어 보는 것도 좋습니다
class ReservationAlaramManager(
private val context: Context,
...
) {
private val manager: AlarmManager by lazy { ... }
}| object SettingPreferenceManager { | ||
| private const val KEY_PUSH_ALARM = "KEY_PUSH_ALARM" | ||
| private lateinit var sharedPreference: SharedPreferences | ||
| fun inIt(context: Context) { |
|
|
||
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| setContentView(R.layout.activity_main) | ||
| addFragment(ReservationListFragment()) | ||
| SettingPreferenceManager.inIt(this) |
There was a problem hiding this comment.
object에 대한 초기화를 담당하는 것이라면, Application 클래스에서 해 보는건 어떨까요?
Comment on lines
+19
to
+20
| val notificationBuilder = generateNotificationBuilder(notificationManager) | ||
| bindNotification(notificationBuilder, notificationData) |
There was a problem hiding this comment.
NotificationBuilder를 NotificationData에서 만들 수 있지 않을까요?
Comment on lines
+29
to
+30
| title = context.getString(woowacourse.movie.R.string.reservation_notification_title), | ||
| contextText = "${movieUiModel.title} context.getString(woowacourse.movie.R.string.reservation_notification_description)", |
Comment on lines
+6
to
+14
| class PushAlarmSwitch(val view: Switch) { | ||
| init { | ||
| SettingPreferenceManager.inIt(view.context) | ||
| view.isChecked = SettingPreferenceManager.getAlarmReceptionStatus() | ||
| view.setOnClickListener { | ||
| SettingPreferenceManager.changeAlarmReceptionStatus() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
PushAlaramSwitch는 클래스로 분리된 장점이 없습니다.
단순히 private 함수로 존재할 내용이 클래스로 만들어진 차이만 가지고 있어요.
실제로 PushAlarmSwitch의 역할을 하는 것으로 기대한다면,
Switch를 상속받은 커스텀 뷰를 구현하는게 맞습니다.
다만 현재 미션에서의 요구사항은 아니기에 참고만 해 주시면 좋겠습니다.
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.
안녕하세요 안드로이드 5기 스캇입니다!
오랜만에 뵙는 것 같아요!! 반갑습니다!!
이번 리뷰 잘 부탁드립니다😀
이번 단계 변경사항은 다음과 같습니다😀
https://github.com/woowacourse/android-movie-theater/pull/23/files/099110635a845df97d9451f12155e9485385c125..d0c5c666635b3bca646ee2c45cf2106e3d6d4c63