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

[스캇] 3, 4 단계 영화 티켓 예매 제출합니다. #39

Merged
merged 92 commits into from
Apr 27, 2023

Conversation

chws0508
Copy link

안녕하세요, 스캇입니다!

이번에 도메인 구조를 많이 바꾸다 보니 커밋을 제대로 하지 못하였네요... 죄송합니다 ㅠ
이번 단계에서는 객체지향적으로 최대한 확장성 있는 구조로 짜려고 노력해 보았습니다.

그리고 가장 고민이 됐던 점은 view Model 과 domain Model 의 호환성 및 사용 시기 입니다. 저번 단계에서 View와 관련된 행동은 View 모델을 이용하시라는 리뷰를 남기셨기 때문에, 이번 단계에서 최대한 비즈니스 로직이 필요한 경우에는 Domain model 을 이용하고, View를 그려줄 때는 Mapper를 이용해 Domain Model을 View Model로 바꾸어서 View를 그려주었습니다. 하지만 이 과정에서 상당히 많은 불필요한 코드가 발생한다고 느꼈고, 혹시라도 실수로 Domain model을 View model로 바꾸어주는 작업을 빼먹는다면, ViewModel의 데이터와 Domain Model의 데이터가 달라져서 오류가 발생할 수 있다고 생각이 듭니다. View Model 은 꼭 필요한 것일까요? ViewModel은 그냥 데이터 전달용으로만 쓰고, Domain 모델의 데이터만을 이용하는 것이 더 편리하다는 생각이 드네요. 리뷰어님의 생각이 궁금합니다!

DYGames and others added 30 commits April 11, 2023 14:41
@@ -0,0 +1,3 @@
package woowacourse.movie.view.model

interface ViewModel

Choose a reason for hiding this comment

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

View관련 Model이라 ViewModel로 네이밍을 지으셨군요!
ViewModel은 MVVM패턴의 ViewModel을 연상시키는거 같아요!
Item, State, UiModel 과 같은네이밍은 어떤가요?

finishActivity()
} else {
renderMovieView(movieViewModel)
val movie = getMovie(movieViewModel)

Choose a reason for hiding this comment

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

movieDateTimePicker에 View를 그려주기위해 domain객체로 변환할 필요가 있을까요?
프레젠테이션의 모델에게 위임해보세요~!

Comment on lines 89 to 94
val dateTime = TicketDateTimeViewModel(
LocalDateTime.of(
movieDateTimePicker.getSelectedDate(),
movieDateTimePicker.getSelectedTime()
)
)

Choose a reason for hiding this comment

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

movieDateTimePicker에게 dateTime관련 작업을 위임했다면,
getSelectedDateTime으로 LocalDateTime을 리턴해보면 어떨까요?

private const val COUNTER_SAVE_STATE_KEY = "counter"
private const val DATE_SPINNER_SAVE_STATE_KEY = "date_spinner"
private const val TIME_SPINNER_SAVE_STATE_KEY = "time_spinner"
private const val MOVIE_DATA_NULL_ERROR = "영화 데이터가 들어오지 않았어요!!"

Choose a reason for hiding this comment

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

View에 표시되는 문자열은 리소스에서 관리하면 좋을거같아요!

val movies = MockMoviesFactory.generateMovies()
val advertisementViewModel = MockAdvertisementFactory.generateAdvertisement()
val movieList = findViewById<RecyclerView>(R.id.main_movie_list)
movieList.layoutManager = LinearLayoutManager(this).apply { LinearLayoutManager.VERTICAL }

Choose a reason for hiding this comment

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

XML에서도 layoutManager를 추가해줄수 있어요! app:layoutManager=

import android.widget.TextView
import woowacourse.movie.view.model.SeatRowViewModel

class SeatTable(

Choose a reason for hiding this comment

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

좌석상태을 그려주는 초기값을 생성시점에 주입받게 작업하면 어떤 장점이 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

여러 종류의 SeatTable을 쉽게 만들 수 있다고 생각했습니다

Choose a reason for hiding this comment

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

이미 선택한 좌석이 있는 SeatTable을 그려주는것도 고려해도 좋을거같아요!

import woowacourse.movie.view.model.SeatRowViewModel
import woowacourse.movie.view.model.SeatViewModel

class SeatView(val view: TextView, row: SeatRowViewModel, col: Int, onClick: (SeatView) -> Unit) {

Choose a reason for hiding this comment

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

row은 SeatRowViewModel객체이고,
col은 Int네요 어쩐 기준으로 나눈걸까요?
통일하면 좋을거같아요!

Copy link
Author

@chws0508 chws0508 Apr 24, 2023

Choose a reason for hiding this comment

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

SeatRow 는 알파벳으로 나타내기 때문에, SeatRowViewModel enum 클래스를 따로 만들어서 관리했었습니다.

Comment on lines 33 to 36
private fun getSeat(): Seat {
val seatViewModel = getSeatViewModel()
return SeatMapper.toDomain(seatViewModel)
}

Choose a reason for hiding this comment

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

도메인의 범위는 어딜까요? 필요할때 도메인 객체로 변경해보아요!

Comment on lines 29 to 41
val movieViewModel = MovieMapper.toView(movie)
posterImageView.setImageResource(movieViewModel.picture)
val dateFormat =
DateTimeFormatter.ofPattern(movieDateTextView.context.getString(R.string.movie_date_format))
movieDateTextView.text = movieDateTextView.context.getString(R.string.movie_date).format(
dateFormat.format(movieViewModel.startDate),
dateFormat.format(movieViewModel.endDate)
)
runningTimeTextView.text =
runningTimeTextView.context.getString(R.string.movie_running_time)
.format(movieViewModel.runningTime)
titleTextView.text = movieViewModel.title
reservationButton.setOnClickListener { onClickEvent(movie) }

Choose a reason for hiding this comment

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

그리고 가장 고민이 됐던 점은 view Model 과 domain Model 의 호환성 및 사용 시기 입니다. 저번 단계에서 View와 관련된 행동은 View 모델을 이용하시라는 리뷰를 남기셨기 때문에, 이번 단계에서 최대한 비즈니스 로직이 필요한 경우에는 Domain model 을 이용하고, View를 그려줄 때는 Mapper를 이용해 Domain Model을 View Model로 바꾸어서 View를 그려주었습니다. 하지만 이 과정에서 상당히 많은 불필요한 코드가 발생한다고 느꼈고, 혹시라도 실수로 Domain model을 View model로 바꾸어주는 작업을 빼먹는다면, ViewModel의 데이터와 Domain Model의 데이터가 달라져서 오류가 발생할 수 있다고 생각이 듭니다. View Model 은 꼭 필요한 것일까요? ViewModel은 그냥 데이터 전달용으로만 쓰고, Domain 모델의 데이터만을 이용하는 것이 더 편리하다는 생각이 드네요. 리뷰어님의 생각이 궁금합니다!

이부분은 사실 트레이드 오프의 영역이 있을수 있다고 생각이 들어요!
모든 도메인 모델을 변경해주지 않아도 될거같단 생각이들어요!
모델을 분리하게된 계기가 무엇인지 고민해보아도 좋을거같아요.
도메인 모델의 Serializable가 붙으면서, 도메인 모델이 Ui요소가 들어가기 때문에 분리하기시작했네요!
이처럼 도메인의 모델을 ui에서 쓰다보면, ui요소에 따라 모델이 변경될가능성이 크기도해요

ex> 위의 코드는 매번 bind될때마다 매번 DateTimeFormatter를 돌려주고있는데,
성능을 위해, bind될때마다가 아닌, 초기시점에 문자열을 변경하여 객체에 담도록 수정하게되면
ui에 그려주기위해 모델의 데이터 타입이 추가되곤해요!

각각의 방법에 장단점이 있으니, 고민해보셔도 좋을거같아요!

  • Domain <-> View 로 변경되는 기준을 세워보아요!
    도메인 모듈에 메세지를 보내기전까진 ui로 간주하고 ui모델을 사용해보면 어떨까요?

Copy link
Author

@chws0508 chws0508 Apr 24, 2023

Choose a reason for hiding this comment

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

도메인 모델에 유효성 검사를 하는 로직이 있을 수 있다고 생각하기 때문에, UI모델의 값이 변경되거나, UI 모델을 처음 생성할 때, 도메인 로직을 사용해야만 할 때, 이 3가지 경우에 도메인 model로 변경해주고 이 외에는 ui 모델을 사용하는 것으로 기준을 잡았습니다!

Comment on lines 25 to 31
internal fun `티켓을 삭제할 수 있다`() {
// given
val ticketOffice = TicketOffice(tickets = Tickets(listOf()), peopleCount = 3)
// when
ticketOffice.addTicket(makeTestTicket(3, 3))
ticketOffice.addTicket(makeTestTicket(3, 4))
ticketOffice.deleteTicket(makeTestTicket(3, 4))
Copy link

@namjackson namjackson Apr 23, 2023

Choose a reason for hiding this comment

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

deleteTicket 테스트를 수행하지만,
준비를 위해 addTicket가 실행되고있어요,
deleteTicket 테스트가 addTicket에 의존적이진 않을까요?
TicketOffice(tickets = 을 통해 주입방식을사용해보면어떨까요?

Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

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

안녕하세요 스캇!
3,4단계 미션 고생하셨어요,
잘구현해주셨네요! 👍
몇가지 고민할법한 포인트들 코멘트로 남겼으니, 확인해주세요!

Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

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

안녕하세요 스캇 !
피드백 적용 고생하셨어요!
추가적인 피드백 남겼으니, 확인해주세요!

private val peopleCountTextView: TextView by lazy { findViewById(R.id.movie_reservation_result_people_count) }
private val seatTextView: TextView by lazy { findViewById(R.id.movie_reservation_result_seat) }
private val priceTextView: TextView by lazy { findViewById(R.id.movie_reservation_result_price) }
private val ticketOfficeUiModel: TicketOfficeUiModel by lazy {

Choose a reason for hiding this comment

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

ticketOfficeUiModel은 필요할까요?
모든 모델을 uiModel에 작성해주지 않으셔도 됩니다!

ReservationResult에는 Tickets나 Reservation만 넘겨주어도 좋지않을까요?
TicketOffice는 티켓 예약을 도와주는 객체가 아닐까요?

Comment on lines 61 to 65
val ticketOffice = TicketOfficeMapper.toDomain(ticketOfficeUiModel)
renderDate()
renderPeopleCount()
renderSeatInformation()
renderPrice(ticketOffice)

Choose a reason for hiding this comment

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

renderPrice를 위한 작업은 uiModel에서 처리해도 좋을거같아요!

Comment on lines 82 to 84
(seatTextView.text.toString() + ticket.seat.row + ticket.seat.col)
if (index != ticketOfficeUiModel.ticketCount - 1) seatTextView.text =
seatTextView.text.toString() + ", "

Choose a reason for hiding this comment

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

이러한 로직들을 UiModel에 위임할수 있을거같아요!


import domain.Seat

class RankBPolicy (

Choose a reason for hiding this comment

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

Rank의 종류가 추가/변경도 고려하면 좋을거같아요!
특히 RankPolicy의 경우에는 사실, Rank의종류와 star,end만 다른거 같아요!
Rank들도 따로 관리하면 어떨까요?

Copy link
Author

@chws0508 chws0508 Apr 25, 2023

Choose a reason for hiding this comment

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

Rank 클래스 내에서 Rank 를 반환하도록 수정하였습니다. 하지만 만약 랭크를 정하는 조건이 현재는 좌석의 개수가 고정되있기 떄문에 고정된 row 열값으로 랭크를 판단할 수 있지만, 영화관의 좌석의 크기가 바뀌고, 랭크를 정하는 조건이 복잡해진다면, 어떻게 확장시킬 수있을지 고민이됩니다.

Choose a reason for hiding this comment

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

사실 구현은 정답은 없는 부분이니, 고민만해주셔도 좋아요!
Rank*Policy 클래스가 중복된 코드가 많아보여 남긴 코멘트였어요!
SeatRank, StartRow, EndRow를 생성자로 가진 열겨형이나 sealed를 활용해도 좋을거같아요!

private fun changeSeatViewState(seatView: SeatView) {
val ticket =
ticketOffice.generateTicket(
ticketDateTime,

Choose a reason for hiding this comment

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

peopleCount는 생성자에서 받는데, date는 함수에서 받는 이유가 있을까요?

Copy link
Author

@chws0508 chws0508 Apr 25, 2023

Choose a reason for hiding this comment

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

ticket에게 필요한건 date와 Seat 이기 때문에 이 둘을 받아서 Ticket을 생성한다. 라는 의미를 보여주고 싶었습니다! 그런데 생각해보니 TicketOffice 가 date 를 가지고 있어도 어색하지 않을 것 같아서 ticketOffice 가 date 를 가지고 있도록 하였습니다.

Comment on lines 116 to 120
if (ticketOffice.tickets.isContainSameTicket(ticket)) {
setViewNotSelected(seatView, ticket)
} else {
setViewSelected(seatView, ticket)
}

Choose a reason for hiding this comment

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

ticketOffice의 데이터에 접근하고, deleteTicket, addTicket을 화면단에서 처리되기 보단,
ticketOffice에서 예매의 결과값을 받아서 위 로직을 진행하면 어떨까요?
ticketOffice에게 좀더 위임해도 좋을거같아요!

import android.widget.TextView
import woowacourse.movie.view.model.SeatRowViewModel

class SeatTable(

Choose a reason for hiding this comment

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

이미 선택한 좌석이 있는 SeatTable을 그려주는것도 고려해도 좋을거같아요!

@namjackson
Copy link

3,4단계는 시간이 금방간거 같네요 ㅠㅠ! 코멘트를 많이 못드린거같아 아쉽네요!
티켓예매 미션은 여기서 종료하도록할게요! 다음 미션들도 응원하겠습니다! 💪

Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

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

안녕하세요 스캇!
티켓 예매 미션 고생하셨습니다!
추가적인 코멘트는 다음 미션 수행하시면서 고민해보셔도 좋을거같아요!
티켓 예매 미션은 이대로 종료하겠습니다! 고생하셨습니다!

Comment on lines +7 to +10
fun renderSeatInformation(textView: TextView) {
textView.text = (textView.text.toString() + row + col)
}

Choose a reason for hiding this comment

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

커뮤니케이션 미스가 있었나봐요 ㅠㅠ!

UI 레이어이긴하지만, Model 역할과는 조금 다른거같아요!
UI 모델에서 직접적인 textView를 받아서 값을 넣어주기보단,
UI에 넣어줄 값을 반환하는 방향을 고민해보아요!
toString 등과 같은 함수나, 변수를 활용해보아요!

Comment on lines +24 to +43
fun renderMovie(
posterImageView: ImageView? = null,
titleTextView: TextView? = null,
dateTextView: TextView? = null,
runningTimeTextView: TextView? = null,
descriptionTextView: TextView? = null
) {
posterImageView?.setImageResource(picture)
titleTextView?.text = title

val dateFormat =
DateTimeFormatter.ofPattern(titleTextView?.context?.getString(R.string.movie_date_format))
dateTextView?.text = dateTextView?.context?.getString(R.string.movie_date)?.format(
dateFormat.format(startDate), dateFormat.format(endDate)
)
runningTimeTextView?.text =
runningTimeTextView?.context?.getString(R.string.movie_running_time)
?.format(runningTime)
descriptionTextView?.text = description
}

Choose a reason for hiding this comment

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

전 코멘트랑 모두 동일해요 ㅠㅠ!
이런 함수는 모델의 책임이라기 보단, View의 책임에 가까워요,
MovieUiModel을 사용하는 다른 UI 관련곳에서 재활용할수 있을까요?

Comment on lines +104 to +109
val ticket = ticketOffice.generateTicket(
SeatUiModel.toNumber(seatView.row), seatView.col
)
ticketOffice.updateTickets(ticket)
val ticketsUiModel = TicketsMapper.toUi(ticketOffice.tickets)
seatTable.updateTable(ticketsUiModel)

Choose a reason for hiding this comment

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

모든 모델을 uiModel로 변경하지 않으셔도 좋아요 👍

ticket을 생성하고, 생성된 값을 업데이트해주기보다,
ticketOffice에게 seat정보를 포함한 메세지를 보내서 위임해보아도 좋을거같아요!

Choose a reason for hiding this comment

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

추후에는 TicketsMapper를 확장함수 형태로 사용하셔서 가독성을 늘려보셔도 좋을거같아요!
ticketOffice.tickets.toUiModel()


import domain.Seat

class RankBPolicy (

Choose a reason for hiding this comment

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

사실 구현은 정답은 없는 부분이니, 고민만해주셔도 좋아요!
Rank*Policy 클래스가 중복된 코드가 많아보여 남긴 코멘트였어요!
SeatRank, StartRow, EndRow를 생성자로 가진 열겨형이나 sealed를 활용해도 좋을거같아요!

@namjackson namjackson merged commit 0de678c into woowacourse:chws0508 Apr 27, 2023
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.

3 participants