[부나] 3, 4단계 영화 극장 선택 제출합니다. #33

tmdgh1592 · 2023-05-07T00:12:44Z

안녕하세요 리뷰어님!
이번 피드백도 잘 부탁드립니다 😃

미션을 진행하면서 여쭤보고 싶은 내용이 있습니다.
Databinding 리팩토링을 진행하다보니, Presenter가 가지고 있는 UI 모델을 View(xml)에서 꺼내서 binding 해주는 구조가 되었습니다.
기존에는 View에서 Presenter에게 이벤트를 알리고, Presenter가 View에게 데이터를 메서드 인자로 전달하는 구조였습니다.
ex)

// Contract.View.kt
fun showTicketingResult(reservation: Reservation)

// Contract.Presenter.kt
fun onXxx()

위 코드에서처럼 View에서 Presenter.onXxx() 메서드를 호출하면 Presenter에서 View.showTicketingResult(...)을 호출해주는 티키타카 형식이었습니다.
그런데 지금은 데이터를 바인딩 해야 한다는 이유로 아래와 같은 구조가 되었습니다.

// Contract.Presenter.kt
fun getReservation(): Reservation

또한, 화면 회전과 같은 상황에서 ui 상태 저장을 위해 presenter의 객체를 꺼내와서 저장해야 하는 경우에도 위 코드처럼 getter를 구현하게 됩니다..!
presenter에서 값을 반환하지 말고 view에게 메시지를 던지는 형태여야 한다고 알고 있는데, 이런 상황에서는 어떻게 하면 좋을지 모르겠습니다 🫠

MVP 아키텍쳐에 Databinding을 적용하면 이러한 구현 방식이 일반적인 것인지,
또는 다른 좋은 방법이 있는지도 궁금합니다! 🤔

- Reminder -> Notifiable

- 동일한 pushId면 기존 알림이 제거되는 이슈 해결

- 서로 다른 키값을 전달하고 있었음.

- getState() - getState() ...

This reverts commit d071f9a.

# Conflicts: # app/src/main/java/woowacourse/movie/presentation/views/ticketingresult/TicketingResultActivity.kt # app/src/main/java/woowacourse/movie/presentation/views/ticketingresult/contract/TicketingResultContract.kt

tmdgh1592 · 2023-05-08T10:28:32Z

안녕하세요 잭슨!
제가 착각을 해서 리뷰 요청 버튼을 누르지 않았었네요 🥲

…있게 변경

namjackson · 2023-05-09T09:38:07Z

app/src/main/java/woowacourse/movie/presentation/ui/main/contract/presenter/MainPresenter.kt

+
+    override fun changeHistoryState() {
+        state = state.copy(isShownHistory = true, latestScreen = MainScreenState.History)
+        val a = this::changeHomeState


불필요한 코드는 제거해주세요!

테스트를 해보려다가 깜빡하고 남겨놨네요..!
감사합니다!

namjackson · 2023-05-09T09:52:59Z

app/src/main/java/woowacourse/movie/presentation/ui/main/fragments/home/HomeFragment.kt

+    override fun setupAdView(ads: List<ListItem>) {
+        binding.rvAdapter = MovieListAdapter(adTypes = ads)
+    }


HomeFragment 로딩이 안되는 버그가 있네요!
데이터와 상관없는 adapter등의 View의 초기화는 View에서 처리해주세요!

Ads와 movies를 섞어주는 부분도 Presenter에 위임해보면 어떨까요?

Ads와 Movies를 섞어주는 기능을 Presenter가 수행한다면,
RecyclerView는 단순히 데이터 리스트를 보여주는 형태로 구현할 수 있겠네요~!

namjackson · 2023-05-09T09:54:22Z

...va/woowacourse/movie/presentation/ui/main/fragments/home/contract/presenter/HomePresenter.kt

+        view.setupAdView(Ad.provideDummy())
+    }
+
+    override fun loadMoreMovies(size: Int) {
+        val newLoadedMovies = Movie.provideDummy(size)


더미 데이터라도, 싱글턴으로 접근하기보단, 생성자로 주입받아서 사용해보면 어떨까요?
HomePresenter와의 의존성도 줄이고,
HomePresenterTest 작성하기 좋을거같아요!

Movie 또한 Repository와 간단한 Mock Datasource를 만들어서 제공해주도록 변경하였습니다~!

namjackson · 2023-05-09T09:56:39Z

...rc/main/java/woowacourse/movie/presentation/ui/main/fragments/theater/TheaterPickerDialog.kt

+    override fun showTheaterList(items: List<ListItem>) {
+        binding.rvAdapter?.appendAll(items)
+    }


showTheaterList라는 함수지만,
appendAll을 해주고 있네요,
데이터 관리는 presenter 해주기때문에,
replaceList를 해주면 어떨까요?

확실히 appendAll보다는 replaceList로 새로고침 해주는 것이 더 적절한 네이밍과 상황인 것 같습니다!
fun �showTheaterList(...) 만 봤을 땐 영화 목록을 보여주는 것 같지만,
실상 추가해주고 있네요?!

namjackson · 2023-05-09T09:58:04Z

...wacourse/movie/presentation/ui/main/fragments/theater/contract/presenter/TheaterPresenter.kt

+    override fun loadTheaterList() {
+        val newTheaters = theaterRepository.getAllByMovieId(movie.id)
+        theaters.addAll(newTheaters)
+
+        view.showTheaterList(newTheaters.map { it.toPresentation() })
+    }


호출될때마다 theaters가 중복되어 커지네요,
loadTheaterList의 동작을 예상할수 있을까요?
add가 아닌 clear and add 로 해주면 어떨까요?

생명주기에 따라 동일한 극장을 여러개 가지고 있을 수도 있겠군요!
기존 극장 목록을 clear하고 새로 add하는 형식으로 변경하였습니다 : )

namjackson · 2023-05-09T10:02:48Z

...ava/woowacourse/movie/presentation/views/ticketingresult/contract/TicketingResultContract.kt

+            view ?: throw IllegalStateException("View is not attached")
+
+        abstract fun onShowMainScreen()
+        abstract fun getReservation(): Reservation


onSaveInstanceState에서 presenter.cacheState호출 -> View.saveState(데이터)
해당 방법은 조금 불필요해보일수 있긴하겠네요,

저는 개인적으로
캐싱해주는 클래스를 Presenter 생성시점에 주입하거나,
단순하게 화면 회전을 위한 View의 캐싱이면, View에서만 처리해주어도 좋을거같단 생각이 들어요!

namjackson

안녕하세요 부나!
피드백 잘적용해주셨네요
몇가지 추가적인 피드백 남겼으니, 확인해주세요 😄

namjackson · 2023-05-09T10:11:16Z

app/src/main/java/woowacourse/movie/presentation/ui/seatpicker/presenter/SeatPickerPresenter.kt

+    override fun isAllPicked(): Boolean =
+        !pickedSeats.canPick(ticketingState.ticket.toDomain())


외부에서 사용하지 않는건 private을 해주세요! ( interface에 정의하지 않아도 될거같아요!)

namjackson · 2023-05-09T10:31:37Z

app/src/main/res/layout/activity_seat_picker.xml

+        <variable
+            name="presenter"
+            type="woowacourse.movie.presentation.ui.seatpicker.presenter.SeatPickerPresenter" />


이제 사용하지 않게된거같아요!

예약이 진행이 안되는 버그가 있네요!

눌렀을 때 dialog가 보이지 않는 버그가 있네요!
해당 버그를 수정하였습니다 : )

namjackson · 2023-05-09T10:34:02Z

app/src/main/java/woowacourse/movie/presentation/ui/seatpicker/presenter/SeatPickerPresenter.kt

+    override fun reserve() {
+        view.showTicketingConfirmDialog()
+    }


데이터와 상관없는 View 조작이라면, presenter를 사용할필요가 없을거같아요!

엇..! DM 드린 내용과 동일하게 궁금한 점입니다!

지금 당장 reserve()를 View -> Presenter -> View 흐름이 번거로워 보일 수 있지만,
이후에 이와 관련된 비즈니스 로직을 처리할 일이 생긴다면, 조금 더 유연하게 대처할 수 있을 것 같습니다!
(현재 reserve() 라는 네이밍에도 혼동 여지가 있어 보여서 어떻게 수정해야 할지 고민이기도 합니다..)

이런 구조도 trade-off 영역 중 하나라고 생각하고 조금 더 이야기를 나눠보고 싶습니다~!

namjackson

안녕하세요 부나!
피드백 잘적용해주셨네요
몇가지 추가적인 피드백 남겼으니, 확인해주세요 😄

tmdgh1592 · 2023-05-11T14:42:00Z

안녕하세요 잭슨!
미션 시작과 동시에 피드백을 반영하다보니 늦었습니다.. 🙏🥹

함께 이야기 나눠보고 싶은 내용이 조금 길어서 DM으로 남겼습니다!
감사합니다 : )

tmdgh1592 · 2023-05-11T15:00:48Z

onSaveInstanceState에서 presenter.cacheState호출 -> View.saveState(데이터)
해당 방법은 조금 불필요해보일수 있긴하겠네요,

아직 제시해주신 방법에 대해 이해가 잘 되지 않았습니다..

onSaveInstanceState(bundle: Bundle)의 Bundle 없이,
View.saveState(데이터) 라는 메서드로 어떻게 UI 상태를 저장할 수 있는지 잘 모르겠습니다..!
혹시 조금만 더 자세히 알려주실 수 있으실까요..?!

namjackson

안녕하세요 부나!
영화 예매 미션 고생하셨어요!
피드백 적용 잘해주셨네요,
몇가지 참고하실만한 코멘트 남겼으니, 확인주세요
다음 미션 진행핫실때 참고하면 좋을거같아요!
미션은 이대로 종료하도록할게요! 고생많으셧습니다 😄

namjackson · 2023-05-12T07:11:08Z

.../java/woowacourse/movie/presentation/ui/main/fragments/theater/adapter/TheaterListAdapter.kt

+    fun replaceList(newItems: List<ListItem>) {
+        items.clear()
+        items.addAll(newItems)
+        notifyItemRangeInserted(items.size - newItems.size, newItems.size)


newItems이 더 크다면 음수가 나오네요!
이부분 조심하면 좋을거같네요
미션범위는 모르겟지만, 다음미션 진행시에는 DiffUtil를 학습해보셔도 좋을거같아요 😄

그 부분은 미처 생각하지 못하고 구현했네요 😅
다음 미션에서 diffUtil을 학습하고 적용해보겠습니다!

namjackson · 2023-05-12T07:34:49Z

app/src/main/java/woowacourse/movie/presentation/ui/ticketing/TicketingActivity.kt

+        super.onSaveInstanceState(bundle)
+        bundle.putParcelable(TICKETING_STATE_KEY, presenter.getState())


onSaveInstanceState(bundle: Bundle)의 Bundle 없이,
View.saveState(데이터) 라는 메서드로 어떻게 UI 상태를 저장할 수 있는지 잘 모르겠습니다..!
혹시 조금만 더 자세히 알려주실 수 있으실까요..?!

savedStateRegistry를 통해 SavedStateRegistryController를 접근할수 있긴하네요
savedStateRegistry.performSave
개인적으로 추천하는 방법은 아닌거 같아요!
onSaveInstanceState함수에서 저장하거나,
Presenter를 통해 저장한다면, 다른 캐싱객체를 만들어서 사용할거같아요

namjackson · 2023-05-12T07:43:00Z

app/src/test/kotlin/activities/main/contract/presenter/MainPresenterTest.kt

+    @Test
+    internal fun 마지막_화면_상태가_히스토리일_때_예매_내역_화면_을_보여준다() {
+        // given
+        val state = mockk<MainState>(relaxed = true)


mockk은 주로 생성하기 어렵거나, android종속적인 초기화가 필요할때 ex> context가 필요한 생성자
등에 자주 사용합니다!
이부분의 MainState는 mockk을 사용하지 않는편이 테스트학기 쉬울거같아요!

무조건 Mock 객체를 만드는 것이 좋지만은 않군요..!
필요한 상황을 더 공부해보고 시기적절하게 사용하는 연습을 해보겠습니다 : )

namjackson · 2023-05-12T07:43:41Z

...urse/movie/presentation/ui/main/fragments/history/contract/presenter/HistoryPresenterTest.kt

+        verify(exactly = 1) { view.showMoreHistory(any()) }
+    }
+
+//    @Test


추후에는 테스트코드를 완성해보면 울거같아요!

namjackson · 2023-05-12T07:44:48Z

...urse/movie/presentation/ui/main/fragments/theater/contract/presenter/TheaterPresenterTest.kt

+        val theaterRepository: TheaterRepository = mockk()
+        val movieIdSlot = slot<Int>()
+        every { theaterRepository.getAllByMovieId(capture(movieIdSlot)) } answers { listOf() }
+
+        presenter = TheaterPresenter(movie, theaterRepository)


setUp에서 하면 좋을걱같아요!

namjackson · 2023-05-12T07:45:29Z

...urse/movie/presentation/ui/main/fragments/theater/contract/presenter/TheaterPresenterTest.kt

+        view = mockk(relaxed = true)
+        presenter = TheaterPresenter(
+            movie = mockk(relaxed = true),


View처럼 새성하기 어려운 객체에서만 mockk을 활용하면 좋을거같아요 👍

tmdgh1592 added 30 commits April 30, 2023 19:53

refactor(TicketingResultActivity): intent() 함수명 변경

27b9fc2

refactor(LocalDataStore): 불필요한 null 체크 제거

7703b5f

refactor(Reminder): Reminder 클래스명 변경

24e5dfe

- Reminder -> Notifiable

fix(ReservationNotification): pushId를 고정값 1000에서 data의 해시코드로 변경

4f7d7e7

- 동일한 pushId면 기존 알림이 제거되는 이슈 해결

refactor(Movie): 예약하는 기능을 Movie 클래스로 이동

07d393a

refactor(HistoryFragment): MVP 아키텍쳐로 리팩터링

3b02eb8

refactor(HomeFragment): MVP 아키텍쳐로 리팩터링

093980c

refactor(HomeFragment): MVP 아키텍쳐로 리팩터링

8a13760

refactor(SettingFragment): MVP 아키텍쳐로 리팩터링

b6f1982

refactor(TicketingResult): MVP 아키텍쳐로 리팩터링

1a96aa3

refactor(TicketingActivity): MVP 아키텍쳐로 리팩터링

a8bbe5d

fix(SeatPickerActivity): intent extra data 가져오지 못하는 버그 수정

d384917

- 서로 다른 키값을 전달하고 있었음.

refactor(Ticketing): 뷰와 프레젠터 메서드와 파라미터 변경

6590058

- getState() - getState() ...

refactor(Ticketing): View의 initView()에 movie 인자 추가

0242535

refactor(Ticketing): view를 private으로 변경

8aa32a1

refactor(HistoryFragment): presenter에 view를 attach하도록 변경

d071f9a

refactor(Ticketing): view를 private으로 변경

ebadeee

Revert "refactor(HistoryFragment): presenter에 view를 attach하도록 변경"

a814bcc

This reverts commit d071f9a.

refactor(HistoryFragment): presenter에 view를 attach하도록 변경

756b8d7

refactor(SeatPicker): MVP 아키텍쳐로 리팩터링

a6d2cbb

refactor(SeatPicker): MVP 아키텍쳐로 리팩터링

d237a93

refactor(CacheDataStore): DataStore 이름명 변경

8fc7e43

chore: Data 모듈에 Domain 의존성 추가

76bbcf9

feat: Reservation 데이터 모델 구현

66c1ebf

feat: Data, Domain mapper 구현

dbff0c0

feat(HistoryDao): 영화 예약 정보를 저장하는 기능 구현

4b74487

refactor(HomeFragment): Presenter detach() 호출하도록 변경

b781e81

feat(ReservationDao): 예매 내역을 불러오는 기능 구현

04e676c

feat(SeatPickerPresenter): 메서드 배치 순서 이동

ae60ccc

feat(HistoryFragment): 영화 예매시 예매 내역에 추가되는 기능 구현

118ab25

tmdgh1592 added 3 commits May 8, 2023 17:23

refactor(TicketingResult): Presenter에서 Reservation을 반환하지 않도록 변경

7e57f3a

refactor(activity_main): Presenter 대신 람다를 전달받도록 변경

1a85bee

Merge remote-tracking branch 'origin/step4' into step4

e5a4189

# Conflicts: # app/src/main/java/woowacourse/movie/presentation/views/ticketingresult/TicketingResultActivity.kt # app/src/main/java/woowacourse/movie/presentation/views/ticketingresult/contract/TicketingResultContract.kt

tmdgh1592 added 11 commits May 8, 2023 22:58

refactor(RecyclerViewBindingAdapter): BindingAdapter 속성을 공통적으로 사용할 수 …

19e13c9

…있게 변경

refactor(HomeFragment): xml에서 presenter 메서드를 호출하도록 변경

dedd320

refactor(TheaterPickerDialog): xml에서 presenter 메서드를 호출하도록 변경

b583b65

refactor(package): views 패키지명을 ui로 변경

eaa8f75

refactor(BaseFragment): 모든 Fragment에 BaseFragment 적용

c77b796

refactor(BaseFragmentDialog): BaseFragmentDialog 적용

4f23a58

fix(TheaterPickerDialog): 아이템 클릭해도 무응답인 버그 수정

0a4188c

refactor(TicketingActivity): BaseActivity 적용

7950fed

refactor(TicketingResultActivity): BaseActivity 적용

6eeec45

refactor(SeatPickerActivity): BaseActivity 적용

2df6306

refactor(HistoryFragment): HistoryFragment 반환하는 함수 구현

5ed7a8d

namjackson reviewed May 9, 2023

View reviewed changes

namjackson requested changes May 9, 2023

View reviewed changes

tmdgh1592 added 6 commits May 11, 2023 21:42

refactor(MainPresenter): 불필요한 코드 제거

8b8be46

refactor(MovieRepository): 영화 목록을 레퍼지토리에서 가져오도록 변경

484f541

refactor(TheaterPickerDialog): showTheaterList() call할 때 극장을 새로고침하도록 변경

261fb21

refactor(TheaterPresenter): 극장 목록을 가져올 때, 기존 극장 목록을 초기화하고 새롭게 추가하도록 변경

1602f18

refactor(SeatPickerPresenter): isAllPicked() 메서드를 private으로 변경

cfb076e

fix(SeatPickerActivity): 예약 버튼을 클릭해도 다이얼로그가 보이지 않는 버그 수정

a90d451

refactor(HomePresenter): Presenter에서 영화 아이템 목록에 광고를 추가해서 반환하도록 변경

fcbdb0f

fix(MainActivity): MainActivity에서 처음 시작할 때 HomeFragment 화면이 보이지 않는 버그 수정

9fe5402

namjackson approved these changes May 12, 2023

View reviewed changes

namjackson merged commit a2c00ca into woowacourse:tmdgh1592 May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[부나] 3, 4단계 영화 극장 선택 제출합니다. #33

[부나] 3, 4단계 영화 극장 선택 제출합니다. #33

tmdgh1592 commented May 7, 2023 •

edited

Loading

tmdgh1592 commented May 8, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

namjackson left a comment

namjackson May 9, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023

namjackson May 9, 2023

tmdgh1592 May 11, 2023 •

edited

Loading

namjackson left a comment

tmdgh1592 commented May 11, 2023

tmdgh1592 commented May 11, 2023

namjackson left a comment

namjackson May 12, 2023

tmdgh1592 May 12, 2023

namjackson May 12, 2023

namjackson May 12, 2023

tmdgh1592 May 12, 2023

namjackson May 12, 2023

namjackson May 12, 2023

namjackson May 12, 2023

		override fun isAllPicked(): Boolean =
		!pickedSeats.canPick(ticketingState.ticket.toDomain())

		super.onSaveInstanceState(bundle)
		bundle.putParcelable(TICKETING_STATE_KEY, presenter.getState())

[부나] 3, 4단계 영화 극장 선택 제출합니다. #33

[부나] 3, 4단계 영화 극장 선택 제출합니다. #33

Conversation

tmdgh1592 commented May 7, 2023 • edited Loading

tmdgh1592 commented May 8, 2023

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

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

tmdgh1592 May 11, 2023 • edited Loading

Choose a reason for hiding this comment

namjackson left a comment

Choose a reason for hiding this comment

tmdgh1592 commented May 11, 2023

tmdgh1592 commented May 11, 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

tmdgh1592 commented May 7, 2023 •

edited

Loading

tmdgh1592 May 11, 2023 •

edited

Loading