[아크] 1, 2단계 영화 티켓 예매 제출합니다. #2
Conversation
|
|
||
| startActivity(Intent(this, MovieListActivity::class.java)) |
There was a problem hiding this comment.
MainActivity에서 MovieListActivity를 띄우는 이유가 궁금헤요 :)
앱을 실행 후 뒤로가기를 누르면 MainActivity가 남아있습니다.
어떤 의도인지 잘 모르겠네요.
There was a problem hiding this comment.
MainActivity에서는 추후에 데이터를 미리 로드 할 때 용도로 사용하려고 하였습니다!
지금은 바로 영화 목록들을 받아오도록 수정하였습니다!
| class MovieListAdapter( | ||
| private val context: Context, |
There was a problem hiding this comment.
정말 외부에서 context를 끌어와야할까요?
힌트는 모든 view에는 context를 가지고 있다는 점이에요 :)
| private val Cinema: Cinema, | ||
| ) : BaseAdapter() { | ||
| override fun getCount(): Int { | ||
| return Cinema.size | ||
| } |
There was a problem hiding this comment.
object 객체를 그대로 활용하면 어떤 장단점이 생길까요?
이후에 영화 목록의 추가/제거가 생기면 어떤 코드들이 생겨나야할까요?
There was a problem hiding this comment.
과도한 포장으로 오히려 추후에 수정하기 힘들 수 있다는 점을 인식하고 지금은 List 형태로 동작하도록 변경하였습니다!
| val view = convertView ?: View.inflate( | ||
| parent?.context, | ||
| R.layout.include_movie_list_item, | ||
| null, | ||
| ) | ||
|
|
||
| val posterView = view.findViewById<ImageView>(R.id.movie_poster) | ||
| val titleView = view.findViewById<TextView>(R.id.movie_title) | ||
| val releaseDateView = view.findViewById<TextView>(R.id.movie_release_date) | ||
| val runningTimeView = view.findViewById<TextView>(R.id.movie_running_time) | ||
| val reservationButton = view.findViewById<Button>(R.id.movie_reservation_button) | ||
|
|
||
| with(Cinema[position]) { | ||
| posterView.setImageResource(poster) | ||
| titleView.text = title | ||
| releaseDateView.text = DateUtil(context).getDateRange(startDate, endDate) | ||
| runningTimeView.text = context.getString(R.string.movie_running_time).format(runningTime) | ||
|
|
||
| reservationButton.setOnClickListener { | ||
| val intent = Intent(context, MovieReservationActivity::class.java) | ||
| intent.putExtra(MovieReservationActivity.KEY_MOVIE_SCHEDULE, this) | ||
| startActivity(context, intent, null) | ||
| } | ||
| } |
There was a problem hiding this comment.
뷰를 일일이 가져와야하다보니 코드가 길어질 수 밖에 없겠어요.
그래도 getView 메서드는 이런 상세 내용까지 일일이 알 필요는 없어보여요.
필요한 View를 다루는 객체로 분리해보는 것은 어떨까요?
| with(Cinema[position]) { | ||
| posterView.setImageResource(poster) |
There was a problem hiding this comment.
저는 개인적으로 with 스코프 함수를 좋아하지 않아요.
poster 등의 this에서 참조가능한 MovieSchdule 멤버들을 바로 사용 가능하기 때문인데요,
코드 하이라이트가 보라색으로 똑같이 잡히기 때문에 현재 클래스의 멤버인지, with 스코프 내의 멤버인지 구분이 어렵더라구요.
또, 스코프가 중복되다보면 this에 @~ 라벨을 붙여주어야해서 오히려 더 코드가 길어진다고 생각해요.
이것 말고도 정말 다양한 이유로 좋아하지 않지만, 지면이 부족하네요 😅
아크는 어떻게 생각하시나요?
There was a problem hiding this comment.
말씀해주신 with의 단점들을 인식하고 있었지만 중복을 제거하고 싶다는 마음으로 사용하였습니다!
확장성에서 오히려 가독성을 많이 헤칠 수 있다고 생각하고 저도 스코프 함수를 제거하면서 가독성을 높일 수 있게 연습해보겠습니다!
언급해주신 다양한 이유들이 뭐가 있을지 더 생각해보겠습니다!
|
|
||
| object DiscountPolicy : Serializable { |
There was a problem hiding this comment.
실제 세상에는 할인 전략이 셀 수 없지 많지요.
소인, 경로 우대, 직원할인, 카드사 할인, 포인트, 특정 이벤트 등등...
이러한 내용들이 추가된다면 이 객체는 만 줄 단위의 코드가 될 것으로 예상돼요.
각 할인 전략들을 다형성을 활용해보는 것은 어떨까요?
| private fun getMovieDayPolicy(localTime: LocalTime): (Int) -> Int = when { | ||
| isEarlyMorning(localTime.hour) -> { price -> discountMovieDay(price) - EARLY_MORNING_DISCOUNT_AMOUNT } |
There was a problem hiding this comment.
제어할 수 없는 시간을 주입 받아 제어할 수 있게 만드셨네요 👍
domain/src/main/java/movie/Movie.kt
Outdated
| data class Movie( | ||
| val title: String, | ||
| val runningTime: Int, | ||
| val summary: String, | ||
| val poster: Int, |
There was a problem hiding this comment.
도메인에는 view에 해당하는 요소를 가져서는 안됩니다.
android view의 resource ID는 명확한 View의 요소입니다 :)
| @Test | ||
| fun `정상 요금이 된다`() { |
| @Test | ||
| fun `무비 데이랑 조조 할인이 적용 된다`() { |
| data class MovieTicket( | ||
| val title: String, | ||
| val reserveTime: LocalDateTime, | ||
| private val people: List<MovieTicketPerson>, |
There was a problem hiding this comment.
MovieTicketPerson은 어떤 의미인지 알기가 어렵네요. 어떤 것을 의도하신걸까요?
영화 티켓이 구매자를 참조하고 있는 것 같아 신기하다고 생각했는데, 그것도 아닌 것 같아서요.
There was a problem hiding this comment.
추후에 확장 했을 경우에 사람별로 할인을 적용하고자 했었습니다! 말씀하신대로 구매자를 참조하려고 했으나 현재 단계에서는 필요 없다고 판단하여 지금은 제거하였습니다!
|
|
||
| class Screening( |
There was a problem hiding this comment.
말씀해주신대로 티켓반환은 도메인 로직이라고 판단하여 Cinema로 만들어 따로 뺏습니다!!
| movieListView.setOnItemClickListener { parent, _, position, _ -> | ||
| getListener(parent.getItemAtPosition(position) as MovieListType) | ||
| } |
There was a problem hiding this comment.
AdapterView에 내장되어있는 리스너를 활용하셨네요 :)
현재에는 adaptView.setOnItemClickListener로 동작하도록 변경하였습니다!
여기서 질문이 있습니다!
이 방법 말고도 시도 해본 것은 메소드 매개변수나 클래스 멤버 변수로도 전달 하려던 것들이 있습니다!
리뷰어님께서는 어떤 방법을 선호하시는지 궁금합니다!
요건 setOnItemClickListener를 전달하는 방법을 말씀하시는 걸까요?
여기에서는 메서드로 set하는 것 이외에는 가능한 방법이 없어보이네요.
setOnItemClickListener를 활용하면 필요한 Movie를 가져오는 게 번거로워 보여요.
클릭했을 때 원하는 정보를 바로 받을 수 있게 리스너를 만들어서 등록해보는 건 어떨까요?
그렇기 위해서는 Adapter에 달아야겠네요 :)
There was a problem hiding this comment.
Adapter 생성 당시에 주입하여 리스너를 구현하도록 변경하였습니다!!
| private fun getListener(item: MovieListType) { | ||
| when (item) { | ||
| is Screening -> getScreeningListener(item) | ||
| } | ||
| } | ||
|
|
||
| private fun getScreeningListener(screening: Screening) { | ||
| val intent = Intent(this, MovieReservationActivity::class.java) | ||
| intent.putExtra(MovieReservationActivity.KEY_MOVIE_Screening, screening) | ||
| ContextCompat.startActivity(this, intent, null) | ||
| } |
There was a problem hiding this comment.
이름에 대해서 고민해보세요 :)
getListener 이니 Listener를 반환 해야 할 것 처럼 생겼지만 아예 다른 일들을 하고 있네요!
There was a problem hiding this comment.
아직 Listener에 대해서 미숙하여 해당 실수를 한 것 같습니다!! 다음부터는 의도에 맞게 네이밍 하겠습니다!
| override fun getView(position: Int, convertView: View?, parent: ViewGroup?): View = | ||
| getItemView(screeningList[position]).getView(screeningList[position], convertView, parent) | ||
|
|
||
| private fun getItemView(movieListType: MovieListType): MovieListItem = when (movieListType) { | ||
| is Screening -> ScreeningItem | ||
| } |
There was a problem hiding this comment.
이미 getItemView 메서드를 호출함으로써 View를 가져왔음을 기대했는데,
사실 MovieListItem은 View가 아닌, View생성을 매개해주는 역할이었네요.
메서드명으로 객체들의 역할들을 기대하게 되는데, 이름과 다른 역할을 하고있다면 코드의 가독성을 매우 떨어뜨립니다.
MovieListItem는 정말 "영화목록항목" 만을 담당하는 객체가 맞을까요?
There was a problem hiding this comment.
말씀해주신대로 영화목록항목 이상의 역할들이 알게 되어 이름과 맞지 않는 역할들을 정리 하였습니다!
| var posterView: ImageView, | ||
| var titleView: TextView, | ||
| var releaseDateView: TextView, | ||
| var runningTimeView: TextView, |
| class MovieReservationActivity : AppCompatActivity() { | ||
| private val screening by lazy { intent.getSerializableExtra(KEY_MOVIE_Screening) as? Screening ?: throw IllegalArgumentException() } |
There was a problem hiding this comment.
값이 없으면 throw가 발생해야할까요?
앱이 크래시 난다면 사용자는 어떻게 반응할까요?
There was a problem hiding this comment.
throw를 하지 않고 에러 메세지와 함께 접근하지 못하도록 변경하였습니다!
| private fun initMovieView() { | ||
| MovieReservationView( | ||
| posterView = findViewById(R.id.reservation_movie_poster), | ||
| titleView = findViewById(R.id.reservation_movie_title), | ||
| releaseDataView = findViewById(R.id.reservation_movie_release_date), | ||
| runningTimeView = findViewById(R.id.reservation_movie_running_time), | ||
| summaryView = findViewById(R.id.reservation_movie_summary), | ||
| ).bind(this, screening) | ||
| } |
| inner class DateSpinnerListener : AdapterView.OnItemSelectedListener { | ||
| override fun onItemSelected(parent: AdapterView<*>?, view: View?, position: Int, id: Long) { | ||
| initTimeView(LocalDate.parse(dateSpinner.selectedItem.toString())) | ||
| timeSpinner.setSelection(selectedPosition) | ||
| } | ||
|
|
||
| override fun onNothingSelected(parent: AdapterView<*>?) = Unit | ||
| } |
There was a problem hiding this comment.
inner class를 사용하면 어떤 장단점과 차이점이 있을까요?
class의 위치는 companion object 상단에 위치시키는 게 컨벤션입니다 :)
There was a problem hiding this comment.
inner class와 nested class 구분은 nested class는 외부 클래스와 완전히 분리되는 객체이고 inner class는 외부 인스턴스에 속하게 됩니다!
해당 기능을 구현 할 때 �intent를 줄이기 위해서 object 방식이 아닌 inner class를 사용하였었습니다!
하지만 리스너 구현 방식에서는 두개 이상의 오브젝트가 필요 없다고 판단하여 object로 다시 변경하였습니다!
| override fun invoke(price: PricePolicyInfo): PricePolicyInfo = when { | ||
| isEarlyMorning(price.reservationDateTime.hour) -> price.copy(price = price.price - discountPrice) | ||
| else -> price |
There was a problem hiding this comment.
#2 (comment)
비슷한 의견이에요 :)
EarlyMorningPricePolicy 라는 이름만 보고, 실행 가능한 함수임을 누구나 알아볼 수 있을까요?
| movieListView.adapter = MovieListAdapter( | ||
| items = CINEMA_SAMPLE, | ||
| onClickButton = ::navigateToReservation, | ||
| ) | ||
| } | ||
|
|
||
| private fun navigateToReservation(screeningModel: ScreeningModel) { |
There was a problem hiding this comment.
클릭 이벤트를 액티비티에서 주입시키고, 원하는 값을 받을 수 있게 만드셨네요 👍
다만, onButtonClick처럼 on~Click 사이에 명사를 넣어주는 것이 일반적입니다.
ListAdapter의 onItemClickListener를 예시로 들 수 있겠네요 :)
| private val viewHolder: MutableMap<View, MovieListViewHolder> = mutableMapOf() | ||
|
|
There was a problem hiding this comment.
더 이상 tag에 저장하지 않게 되었네요 👍
아래는 구현하지 않고 고민해보세요 :)
- 다른 어뎁터들이 같은 ViewHolder를 공유해볼 수 있을까요?
- View와 ViewHolder를 1대1 매핑해서 관리하지 않고, ViewHolder만을 관리해볼 수 있을까요?
위 내용들은 RecyclerView에서 어떻게 해결했을 지도 잘 참고해보세요 😊
| .bind( | ||
| posterResource = screeningModel.poster, | ||
| title = screeningModel.title, | ||
| date = getScreeningDate(screeningModel.reservationModel), | ||
| runningTime = view.context.getString(R.string.movie_running_time).format(screeningModel.runTime), | ||
| onClickButton = { onClickButton(screeningModel) }, |
| private val contents by lazy { ReservationContents(activityView) } | ||
| private val navigate by lazy { ReservationNavigation(activityView, screeningModel, ::onReservationButtonClicked) } |
There was a problem hiding this comment.
이름을 좀 더 고민해보아요 :)
navigate는 어딘가로 이동시켜줄 것 같은 느낌을 많이 주네요.
View들을 묶은 객체들이니, 마지막엔 View라는 접미사를 붙여봐도 좋겠어요 :)
| private fun initNavigate() { | ||
| run { navigate } | ||
| } |
| fun update(screeningModel: ScreeningModel) { | ||
| posterView.setImageResource(screeningModel.poster) | ||
| titleView.text = screeningModel.title | ||
| screeningDateView.text = getScreeningDate(screeningModel.reservationModel) | ||
| runTimeView.text = view.context.getString(R.string.movie_running_time).format(screeningModel.runTime) | ||
| summaryView.text = screeningModel.summary | ||
| } |
| fun save(outState: Bundle) { | ||
| outState.putInt(KEY_DATE, dateSpinner.selectedItemPosition) | ||
| } | ||
|
|
||
| fun load(savedInstanceState: Bundle) { | ||
| dateSpinner.setSelection(savedInstanceState.getInt(KEY_DATE)) | ||
| } | ||
|
|
There was a problem hiding this comment.
Spinner (view)가 Bundle을 직접 다루는 역할을 가져야 하는 지에 대해 고민해보아요 :)
이 View에 다른 데이터를 보여주고 싶을 때에도, Bundle을 반드시 넘겨야하겠네요!
안녕하세요 말리빈 리뷰어님!
Level 2에서도 리뷰받게 되서 너무 기쁩니다!!!
이번 미션도 잘 부탁드립니다!!!!!!!