[하티] 1, 2단계 영화 티켓 예매 제출합니다. #10
Conversation
- 메인 페이지 (영화 리스트) - 영화 리스트 아이템 레이아웃 - 영화 상세 정보 페이지 - 영화 예매 완료 페이지
| setMovieList(movies) | ||
| } | ||
|
|
||
| private fun getMovies(): List<Movie> { |
There was a problem hiding this comment.
Mock데이터 관리하는 부분을 분리해보면 어떨까요?
추후, 데이터 변경이나, 테스트시에 용이할거 같아요!
|
|
||
| private fun moveToDetailActivity(movie: Movie) { | ||
| val intent = Intent(this, MovieDetailActivity::class.java) | ||
| intent.putExtra("movie", movie) |
There was a problem hiding this comment.
상수로 관리해보면 어떨까요?
사람의 실수도 방지하고, 변경에 유연하게 대처가 되요!
| private fun getMovieFromIntent() = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| intent.getSerializableExtra("movie", Movie::class.java) | ||
| } else { | ||
| intent.getSerializableExtra("movie") | ||
| } as Movie |
There was a problem hiding this comment.
버전 분기처리 👍 👍
-
리턴값을 생략할수도 있지만, 무엇이 리턴되는지 표기해주면 어떨까요? 어떤 장점이 있을까요?
-
intent.getSerializableExtra("movie") as? Movie안전한 타입캐스팅을 사용해주세요 -
getSerializableExtra함수는 nullable한 데이터가 리턴이 되요! null처리 해주면 어떨까요?
There was a problem hiding this comment.
리턴값을 표기해줄 때 제가 생각하는 장점은,
- 코드를 딱 보았을 때 어떤 값을 리턴하는지 명확하게 보이기 때문에 코드를 읽는 사람의 입장에서 코드를 이해하기에 더 편리할 것 같습니다.
- 더불어서 nullable한지, non-nullable한지 명시해줌으로써 두 타입이 혼돈되어 발생할 수 있는 오류를 사전에 방지할 수 있을 것 같습니다.
There was a problem hiding this comment.
getSerializableExtra 가 nullable한 데이터가 반환될 때 null 처리를 어떻게 해주는 것이 좋은 방법인지 고민이 됩니다 🤔
예를 들어, 현재 영화 리스트에서 영화를 클릭하면 해당 영화에 대한 상세 정보가 보여야 합니다.
이 때 만약 값이 제대로 불러와지지 않았다면,
상세 정보 페이지가 빈 페이지로 보이거나, 혹은 오류 상황을 나타내는 데이터가 보이는 것이 좋을까요,
혹은 오류가 발생함을 토스트 메시지 등으로 알린 후에 앱이 종료되는 것이 맞는 걸까요?
이런 경우 어떠한 방식이 일반적인지, 혹은 선호하는 방식이 있으신지 궁금합니다 :)
There was a problem hiding this comment.
섬세한 고민 👍👍
크래시가 나지 않더라도,
화면이 정상적으로 동작하지 않는다면, 해당 화면을 �떠나는게 좋을거같다는 생각이예요!
하지만 앱까지 전체적으로 종료한다면 유저가 당황해할거 같아요!
detail화면에서 오류가 생긴다면, list화면으로만 안내해주어도 좋을거같아요!
There was a problem hiding this comment.
우선 데이터가 null일 경우, 토스트 메시지를 보여준 후 activity가 finish() 되도록 구현하였습니다!
이 방식으로 실행해보면 토스트 메시지는 보이면서 activity는 아예 넘어가지 않는 것처럼 보이네요..!
| outState.putInt("date_position", dateSpinner.selectedItemPosition) | ||
| outState.putInt("time_position", timeSpinner.selectedItemPosition) | ||
| outState.putInt("count", peopleCount.count) |
|
|
||
| private fun LocalDate.format(): String = format(DateTimeFormatter.ofPattern("yyyy.MM.dd")) | ||
|
|
||
| private fun Movie.getRunningTime(): String = "러닝타임: ${runningTime}분" |
There was a problem hiding this comment.
화면에 보여지는 문자열은 string.xml을 활용해주세요!
string.xml를 사용하면 어떤 장점이 있을까요?
There was a problem hiding this comment.
우선, 여러 곳에서 중복되어 사용되는 문자열을 한 곳에서 한 번에 관리할 수 있다는 장점이 있을 것 같아요!
또한 추가적으로 찾아보니, 다국어 지원을 할 수 있다는 장점도 있는 것 같습니다 :)
| fun isMovieDay(): Boolean = date.dayOfMonth in MOVIE_DAYS | ||
|
|
||
| fun isSaleTime(): Boolean { | ||
| return time.hour <= SALE_TIME_STANDARD_MORNING || time.hour >= SALE_TIME_STANDARD_NIGHT | ||
| } | ||
|
|
||
| companion object { | ||
| private val MOVIE_DAYS = listOf(10, 20, 30) | ||
| private const val SALE_TIME_STANDARD_MORNING = 11 | ||
| private const val SALE_TIME_STANDARD_NIGHT = 20 | ||
| } |
There was a problem hiding this comment.
할인정책이 변경될때마다, 코드의 변경이 생길거같아요!
할인에 대한부분을 추상화해보면 어떨까요?
| override fun toString(): String { | ||
| return "%02d:%02d".format(hour, minute) | ||
| } |
| android:text="@string/button_plus" | ||
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:id="@+id/detail_plus_button" | ||
| app:layout_constraintTop_toTopOf="@id/detail_minus_button" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| android:textStyle="bold" | ||
| android:textSize="25sp" /> | ||
|
|
||
| <Button | ||
| android:id="@+id/detail_booking_button" |
There was a problem hiding this comment.
xml 내의 id나 ui/view 관련해서 id 이름를 짓는 보편적인 방법이나 개인적으로 선호하는 방법이 있으신가요?
예를 들어, 레이아웃마다 영화 제목을 출력하는 TextView가 있을 때 id가 중복되지 않게 어떻게 지어야 할지 모르겠습니다.
사실 xml내 id에 대한 네이밍에 대한 부분은 보통 팀이나 프로젝트마다 각각의 네이밍룰이 있기때문에,
팀원과 네이밍룰을 맞춰보는것도 좋을거 같아요!
중요한 부분은 일관성을 유지하는것이라고 생각이 들어요!
detail_booking_button, button_plus 보다는 일관성있는 구조를 고민해보세요!
| import java.time.LocalDate | ||
| import java.time.format.DateTimeFormatter | ||
|
|
||
| class MovieDetailActivity : AppCompatActivity() { |
There was a problem hiding this comment.
activity의 역할을 줄이고 싶은데, 어떻게 어디서부터 줄여야할 지 고민이 됩니다. 혹시 이 과정에서 알아야 할 키워드나 고려해야할 요소가 무엇인지 궁금합니다!
좋은 고민이네요 👍 activity는 activity특성상 크기가 어쩔수 없이 커지기도합니다!
먼저, 비슷한 역할과 책임이있는 부분을 묶어서 분리해보는 작업을 먼저해보면 어떨까요?
| } | ||
| } | ||
|
|
||
| private fun setTimeSpinner() { |
There was a problem hiding this comment.
spinner에 객체를 요소로 보여줄 때, 스피너 아이템에 보이는 부분을 정의하기 위해 객체에서 toString() 함수를 override 하였습니다. 하지만 이런 방식은 도메인에서 view의 역할을 수행한다고 생각이 드는데, spinner의 adapter를 따로 custom해서 사용하지 않으면서 이런 문제를 해결할 수 있는 방법이 있을까요?
LocalTime을 활용하는 방법도 있고, ViewFormat용 객체를 생성해보면 어떨까요?
- key 값들 상수화 - int 상수에 _ 추가해 가독성 개선
- 그에 따라 TicketTime 클래스 삭제 (역할이 없어짐)
|
안녕하세요, 잭슨! |
| @DrawableRes | ||
| val poster: Int, |
There was a problem hiding this comment.
DrawableRes는 AndroidView와 관련된 리소스입니다!
domain레이어에서 가지지 말아야할 정보는 아닐까요?
| import java.io.Serializable | ||
|
|
||
| @JvmInline | ||
| value class PeopleCount(val count: Int = MINIMUM_COUNT) : Serializable { |
| const val KEY_MOVIE = "movie" | ||
| const val KEY_TICKET = "ticket" |
There was a problem hiding this comment.
Key등의 상수값을 모아서 독립적으로 관리하는 부분은
개인적으로 지양하는 편이예요!
프로젝트가 커지다보면, 파일도 커지고, 관리해야할 상수가 많아지면,
상수의 범위도 그렇고 관리가 잘안되곤해요!
필요한곳에서 관리해보면 어떨까요?
|
|
||
| supportActionBar?.setDisplayHomeAsUpEnabled(true) | ||
|
|
||
| val movie: Movie? = intent.getCustomSerializableExtra<MovieModel>(KEY_MOVIE)?.toDomain() |
There was a problem hiding this comment.
getCustomSerializableExtra이후, 바로 domain 레이어의 모델로 변경해주시네요!
MovieModel과 Movie를 분리한것은 무슨이유때문일까요?
Ui관련된 로직을 수행하는 역할도 MovieModel에게 위임해보아요!
| } | ||
|
|
||
| viewHolder.bind(movie) | ||
| viewHolder.button.setOnClickListener { itemButtonClickListener.onClick(position) } |
There was a problem hiding this comment.
viewHolder내 button에게 직접접근하기보단,
ViewHolder에게 위임해보아요!
| import java.io.Serializable | ||
|
|
||
| @Suppress("DEPRECATION") | ||
| inline fun <reified T : Serializable> Intent.getCustomSerializableExtra(key: String): T? { |
There was a problem hiding this comment.
| import java.time.LocalTime | ||
| import java.time.format.DateTimeFormatter | ||
|
|
||
| class MovieDetailActivity : AppCompatActivity() { |
There was a problem hiding this comment.
MovieDetailActivity의 코드가 길어졌네요!
안드로이드 특성상 어쩔수 없는 부분도 있지만,
비슷한 역할과 책임을 하는것들을 찾아서 분리해보아도 좋을거같단생각이 들어요!
(dateTime선택View, Counter선택View 등이 있을거같네요!)
| val ticket = MovieTicket( | ||
| movie.title, | ||
| LocalDateTime.of( | ||
| dateSpinner.selectedItem as LocalDate, | ||
| timeSpinner.selectedItem as LocalTime | ||
| ), | ||
| peopleCount | ||
| ) |
There was a problem hiding this comment.
movie를 통해 ticket을 리턴한다는 게 정확이 어떤 의미인지, 그리고 왜 그렇게 생각하셨는지 알려주실 수 있나요 ~?
movie를 통해 ticket을 만들기에는, 영화 제목 외에는 영화를 보는 인원과 보려는 날짜 및 시간 정보가 추가적으로 필요하기 때문에
별개의 객체로 두어도 괜찮지 않을까 생각이 들었습니다..!
ticket생성을 위해, movie의 프로퍼티에 직접 접근하기보단,
movie와의 협력을 통해 Ticket생성의 책임을 위임해보라는 의미였습니다!
val ticket = movie.buyTicket( time, peopleCount ...
하지만 아직은 크게 영향이 없는부분이기도하고,
하티님의 생각처럼 별도의 객체를 두어도 괜찮을거 같아요!
구현에는 정답이 없는 문제기 때문에, 각각의 장단점을 고민해보셔도 좋을거같아요!
안녕하세요 잭슨 ! 😄
이번 미션에서 리뷰 받게 된 하티라고 합니다. 잘 부탁드립니다!! :)
이번 미션을 진행하면서 몇 가지 궁금증이 있었는데요,
예를 들어, 레이아웃마다
영화 제목을 출력하는 TextView가 있을 때 id가 중복되지 않게 어떻게 지어야 할지 모르겠습니다.toString()함수를 override 하였습니다. 하지만 이런 방식은 도메인에서 view의 역할을 수행한다고 생각이 드는데, spinner의 adapter를 따로 custom해서 사용하지 않으면서 이런 문제를 해결할 수 있는 방법이 있을까요?안드로이드는 익숙하지 않아 아직 부족한 점이 많아
피드백 많이 많이 부탁드려요!!
미리 시간 내어 리뷰를 주심에 감사드립니다! :)