[둘리] 1, 2단계 영화 티켓 예매 제출합니다. #7
Conversation
|
else문을 사용하지말라고 배웠는데 MovieDetailActivity에서처럼 null처리를 위한 else문도 사용해서는 안될까요? |
| val movieDTO: MovieDTO? = intent.getVersionDependentSerializableExtra(Keys.MOVIE_KEY) | ||
| if (movieDTO != null) { | ||
| initMovieDetailView(movieDTO) | ||
| initReservationInfoView(savedInstanceState, movieDTO) | ||
| } else { | ||
| Toast.makeText(this, DATA_LOADING_ERROR_MESSAGE, Toast.LENGTH_LONG).show() | ||
| finish() | ||
| } |
There was a problem hiding this comment.
else문을 사용하지말라고 배웠는데 MovieDetailActivity에서처럼 null처리를 위한 else문도 사용해서는 안될까요?
if(movie == null)로 토스트를 띄우고 finish를 하면 되겠지만, 뒤에 init문에서 movie뒤에 ?를 붙여주고싶지 않아서 else문을 작성했습니다!
개발에 "사용해선 안된다"는 사실 존재 하지 않아요. 코드가 어떻든 보이기는 동일하게 보일 수 있으니까요.
객체지향 생활체조 연습을 위해 제약을 둔 것 뿐이랍니다.
그렇기에 꼭 연습해보세요 :)
레벨 1때에 else 구문을 어떻게 지웠는지 상기해보며 작성해볼까요?
There was a problem hiding this comment.
이제서야 return이 생각나서 else문을 삭제했습니다 ㅎㅎ
| } | ||
|
|
||
| private fun initMovieDetailView(movieDTO: MovieDTO) { | ||
| MovieDetailViewInitializer(movieDTO).run { |
There was a problem hiding this comment.
run 스코프 함수를 선택한 이유가 무엇인가요?
다른 스코프 함수들과 어떻게 다를까요?
There was a problem hiding this comment.
scope function를 살펴보면 두 가지 기준으로 차이점이 나뉩니다.
- 객체 reference를 호출하는 방법이
it인가,this인가 :it- let, also |this- run, with, apply - 반환값이 Lambda의 결과인가, 객체 reference인가 : Lambda result - let, run, with | 객체 reference - apply, also
사실 run을 선택한 이유는 뜻 그대로 실행한다는 의미로 사용했는데요..!
위 기준에서 서로 겹치는 run과 with에 대해 좀 더 살펴보았습니다.
run과 with의 가장 큰 차이점은 (지금 코드에서 사용한 run의 경우) run은 확장함수이나 with는 아니라는 점, 그리고 run은 nullable 변수에서 사용 가능하나 with에서는 그렇지 않다는 점입니다.
with는 "이미 생성된" 객체에 대해 코드 중복을 줄이고 싶을 때, run은 객체 생성 후 "객체 저장 없이" 원하는 값만 받아오고 싶을 때도 사용할 수 있습니다.
따라서, 현재 코드에서는 객체가 nullable이 아니며, return값이 필요 없고 한 객체에 대해 여러 함수를 사용하려고 하고 있기 때문에 의미적으로 with가 보기 좋아보입니다!
There was a problem hiding this comment.
좋은 공부가 되었기를 바라요 :)
조금 추가하면,
with는 사실 나머지 4개와는 다르게 확장함수가 아니랍니다.
그래서 run과의 비교군으로는 잘 맞지 않아요.
사실 run을 선택한 이유는 뜻 그대로 실행한다는 의미로 사용했는데요..!
저도 공감해요.
다만, run의 반환 값은 Lambda의 결과 라는 것이에요.
그렇기에 이 반환 값을 사용하지 않으므로, 적절하지 않다고 생각해요.
저는 이런 경우, 수신 객체를 가지고 또 무언가를 수행하니, also를 주로 활용해요.
확장 함수의 결과가 변환되지도 않기에 활용해요.
확장 함수에 대한 비교로 말씀 드렸는데, 둘리 의견 처럼 with가 더 적절해 보이네요 :)
제 의견은 정답이 아니니, 이런 의견도 있다 정도로 생각해주시면 좋겠어요 😊
다만, 이러한 일들이 정말 스코프 함수가 필요했는 지 더 고민해보면 좋겠어요.
정말 외부에서 일일이 MovieDetailViewInitializer 객체의 메서드를 호출해주어야할까요?
There was a problem hiding this comment.
일일히 호출하기 보다 생성자에서 Movie 데이터를 받아서 한번에 처리할 수 있을 것 같습니다. :)
그리고 한 가지 더 질문드리고 싶은 부분이 있습니다!
inline fun <T, R> with(receiver: T, block: T.() -> R): R {
return receiver.block()
}with를 찾아볼 때 블로그에 반환값이 없다라는 말을 자주 봤는데요!
with의 정의를 보면 반환 타입이 block의 결과인데... 반환은 있는게 맞지만, 이 반환값을 어딘가에 저장해서 사용하지는 않는다는 의미인건가요?
There was a problem hiding this comment.
with의 정의를 보면 반환 타입이 block의 결과인데...
정확히 짚으셨네요!
저는 사실 한글로 된 블로그에 그리 신뢰도가 높지 않아요.
생각보다 다른 블로그의 글을 그대로 옮겨다가 적는 경우가 꽤 많아요.
그렇기에 잘못된 정보도 그대로 옮기는 경우를 많이 보았어요.
저는 둘리가 건강한 검색을 하고 있다고 생각해요.
믿을 건 영어 공식 문서와 실제 코드입니다 🙂
| ReservationInfoViewInitializer(movieDTO).run { | ||
| initCount(savedCount, findViewById(R.id.text_count)) | ||
| initMinusButton(findViewById(R.id.btn_minus), findViewById(R.id.text_count)) | ||
| initPlusButton(findViewById(R.id.btn_plus), findViewById(R.id.text_count)) | ||
| initReserveButton( | ||
| findViewById(R.id.btn_reserve), | ||
| findViewById(R.id.text_count), | ||
| findViewById(R.id.spinner_date), | ||
| findViewById(R.id.spinner_time) | ||
| ) | ||
| initDateSpinner( | ||
| savedDate, | ||
| findViewById(R.id.spinner_date), | ||
| findViewById(R.id.spinner_time) | ||
| ) | ||
| initTimeSpinner( | ||
| savedTime, | ||
| findViewById(R.id.spinner_date), | ||
| findViewById(R.id.spinner_time) | ||
| ) | ||
| } |
| class MovieDetailViewInitializer(private val movieDTO: MovieDTO) { | ||
|
|
There was a problem hiding this comment.
영화 상세에 대한 View를 초기화만 하는 객체를 만들고 싶었던 것으로 보여요.
정말 초기화만 담당한다면, 이후 View에 대한 수정이나 인터렉션등이 추가되면, 어떤 변경점이 생겨날까요?
MovieDetailView
를 만든다고 생각해보고 만들어보세요 :)
| fun initTimeSpinner(savedTimePosition: Int, dateSpinner: Spinner, timeSpinner: Spinner) { | ||
| val times = movieDTO.playingTimes.times[dateSpinner.selectedItem] ?: emptyList() |
There was a problem hiding this comment.
playingTimes의 참조가 길어지는 이러한 형태가 많네요.
매번 호출하는 곳에서 emptyList() 할당이 반복되네요. 줄일 수 있는 방법이 있을까요?
| data class MovieDTO( | ||
| @DrawableRes val image: Int, | ||
| val title: String, | ||
| val playingTimes: PlayingTimes, | ||
| val runningTime: Int, | ||
| val description: String | ||
| ) : java.io.Serializable |
There was a problem hiding this comment.
도메인과 뷰에서 관리할 모델을 분리하셨네요 👍
Movie 객체에서 Serializable 제거도 잘 해주셨네요 :)
다만 DTO는 안드로이드에서 사용하지 않는 용어입니다. spring 진영에서 자주 사용하는 용어예요.
구글 문서 등을 잘 참고해서 다른 종류의 이름을 붙여보세요 :)
이름 짓기는 세상 어려운 일이니 충분히 고민해보세요.
There was a problem hiding this comment.
Model이라는 단어를 사용하여 수정했습니다!
Entity나 ViewModel이라는 네이밍을 사용하는 것 같은데, Entity는 데이터베이스에서 주로 사용되는 용어로 가장 하위 단계인 것 같고 ViewModel이 뷰에 대한 모델이라는 의미에 가까운 네이밍인 것 같습니다.
MovieViewModel은 너무 긴 것 같아 MovieModel로 수정했습니다.
|
|
||
| class PlayingTimes(startDate: LocalDate, endDate: LocalDate) : java.io.Serializable { |
There was a problem hiding this comment.
도메인 모델이기 때문에 필요없다고 판단하여 삭제했습니다!
| data class TicketingInfo private constructor(val title: String, val playingDate: LocalDate, val playingTime: LocalTime, val count: Int, val price: Price, val payment: String) : java.io.Serializable { | ||
| companion object { |
There was a problem hiding this comment.
멤버변수가 많아지면 멤버변수 개수 만큼 줄바꿈을 해주세요 :)
줄여볼 수 있는 방법을 더 고민해볼까요?
There was a problem hiding this comment.
payment가 지금 요구사항과 앞으로의 요구사항을 모두 포함했을 때 모두 '현장'임을 생각했을 때 지워도 되는 프로퍼티라고 판단하여 삭제했습니다!
| @Suppress("DEPRECATION") | ||
| inline fun <reified T : java.io.Serializable> | ||
| Intent.getVersionDependentSerializableExtra(key: String): T? { |
There was a problem hiding this comment.
#7 (comment)
힌트는 Compat 이라는 단어입니다 :)
어떤 의미이며, 안드로이드에서는 어떻게 활용되고 있을까요?
There was a problem hiding this comment.
Compat의 의미가 '호환'이라는 의미였네요!
android developers에서 Compat이라는 단어를 이름에 포함하는 클래스들을 열어보니, 함수 or 클래스명 + 'Compat' 이라는 이름으로 SDK 버전에 따라 분기를 나누어 처리하는 것을 확인할 수 있었습니다!
| object Keys { | ||
| const val MOVIE_KEY = "MOVIE" | ||
| const val COUNT_KEY = "COUNT" | ||
| const val SPINNER_DATE_KEY = "SPINNER_DATE" | ||
| const val SPINNER_TIME_KEY = "SPINNER_TIME" | ||
| const val INFO_KEY = "TICKETING_INFO" | ||
| } |
There was a problem hiding this comment.
저는 이 키들을 데이터를 받아야할 Activity의 상수로 두는 편이에요.
한 곳에서 상수를 관리하는 건 최대한 피하려고 합니다. 모든 파일의 의존관계가 생기고, 나중에 관리하기 어려워지기 때문이에요.
There was a problem hiding this comment.
수신 액티비티에 상수를 둘 생각은 하지 못했네요..!! 감사합니다 :)
|
말씀해주신 피드백과 여러 코드를 보면서 조금 감을 잡은 것 같습니다! (제 착각일 수도 있겠지만요 ㅎㅎ..) |
|
|
||
| class DateSpinnerListener(private val playingTimes: Map<LocalDate, List<LocalTime>>, private val spinnerTime: Spinner) : AdapterView.OnItemSelectedListener { |
There was a problem hiding this comment.
이 스피너 리스너는 다른 스피너에 크게 의존적이에요.
동일한 스피너 리스너를 재활용할 수 있게 할 수 있을까요?
혹은 이 리스너만 따로 클래스로 분리되는 게 적절할지도 고민해보면 좋겠어요 :)
| val movie: MovieModel? = intent.getSerializableExtraCompat(MOVIE_KEY) | ||
| if (movie == null) { | ||
| Toast.makeText(this, DATA_LOADING_ERROR_MESSAGE, Toast.LENGTH_LONG).show() | ||
| finish() | ||
| return | ||
| } | ||
| MovieDetailView(findViewById(R.id.layout_detail_info)).set(movie) | ||
| ReservationInfoView(findViewById(R.id.layout_reservation_info)).set(savedInstanceState, movie) |
There was a problem hiding this comment.
흐름도 분리도 아주 깔끔해졌네요 👍
덕분에 액티비티 라인 수가 아주 줄어들었네요 :)
아래는 조금 어렵지만, 도전해보셔도 좋고 공부만 해보셔도 좋아요 :)
MovieDetailView를 아래 처럼 xml에 바로 세팅해볼 수 있을까요?
<androidx.constraintlayout.widget.ConstraintLayout>
<...MovieDetailView
android:....
android:.... />
<...ReservationInfoView
android:....
android:.... />
</androidx.constraintlayout.widget.ConstraintLayout>| fun set(movie: MovieModel) { | ||
| setImageView(movie.image) | ||
| setTitle(movie.title) | ||
| setPlayingDate(movie.startDate, movie.endDate) | ||
| setRunningTime(movie.runningTime) | ||
| setDescription(movie.description) | ||
| } |
There was a problem hiding this comment.
set을 할 때마다 findViewId를 실행해주어야겠네요.
한 번만 해보는 방법도 있겠네요 :)
| intent.putExtra(TicketResultActivity.INFO_KEY, ticket.toPresentation()) | ||
| it.context.startActivity(intent) |
There was a problem hiding this comment.
이 View 객체를 재활용해야할 때 어떤 문제가 발생할 수 있을까요?
이 View만이 가져야 할 역할 이외의 것은 외부에서 받아보는 건 어떨까요?
| object : MovieListItemListener { | ||
| override fun onClick(movie: MovieModel, view: View) { | ||
| val intent = Intent(view.context, MovieDetailActivity::class.java) | ||
| intent.putExtra(MovieDetailActivity.MOVIE_KEY, movie) | ||
| view.context.startActivity(intent) | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분은 어떤 이유로 setOnClickLisntener { } 처럼 바로 람다로 표현하지 못할 수 밖에 없었을까요? 🤔
| class MovieListAdapter(private val movies: List<MovieModel>, private val listener: MovieListItemListener) : BaseAdapter() { | ||
| private val viewHolders: MutableMap<View, ViewHolder> = mutableMapOf() |
There was a problem hiding this comment.
더 이상 tag에 저장하지 않게 되었네요 👍
아래는 구현하지 않고 고민해보세요 :)
- 다른 어뎁터들이 같은 ViewHolder를 공유해볼 수 있을까요?
- View와 ViewHolder를 1대1 매핑해서 관리하지 않고, ViewHolder만을 관리해볼 수 있을까요?
위 내용들은 RecyclerView에서 어떻게 해결했을 지도 잘 참고해보세요 😊
안녕하세요, 리뷰어님! 1, 2단계 영화 티켓 예매 제출합니다.
날짜 스피너의 setSelection(position, animate) 함수를 사용하면 위치가 바뀌고, setSelection(position) 함수를 사용하면 바뀌지 않는 점을 찾았습니다. 하지만 이유는 찾지 못했습니다.🥲 animate의 값을 false로 주면 onItemSelected 함수가 실행되지 않는 것으로 알고 있는데, 어째서 레이아웃이 변경되는걸까요..? 또한 true로 주더라도 false인 경우와 똑같이 작동하는 것을 보고 혼란스럽습니다.
(그럼에도 불구하고 setSelection(position, animate)을 사용한 이유는, setSelection(position)를 사용하면 화면 회전 시 시간 스피너가 지정한 값이 아닌 리스트의 첫 번째 값으로 초기화되어서 입니다! 기본 요구사항을 맞추기 위해 레이아웃이 변경되지만 값은 유지 되는 해당 함수를 사용했습니다.)
날카로운 피드백 부탁드립니다! 많이 배우겠습니다. :)