영화 목록 출력 #22

handnew04 · 2020-03-08T15:23:12Z

No description provided.

zion830

안녕하세요
몇가지 리뷰 남겨보았습니다 🙂

zion830 · 2020-03-13T18:09:24Z

ToyProject/app/src/main/java/com/example/toyproject/adapter/DataBindingAdapter.kt

+
+@BindingAdapter("bind:replace")
+fun RecyclerView.replaceAll(item: List<SearchMovieData>?) {
+    if (!item.isNullOrEmpty()) (adapter as? MainMoviePostingRecyclerAdapter)!!.setItemList(item as ArrayList<SearchMovieData>)


as?는 타입 캐스팅에 실패했을 경우 null을 반환하기 때문에, 만약 캐스팅이 실패한다면 setItemList가 호출될 때 NPE가 발생하게 됩니다. !! 대신 ?를 사용하는 것이 좋아 보입니다 ㅎㅎ

앗 감사합니다!
8ab4e5e

zion830 · 2020-03-13T18:12:14Z

ToyProject/app/src/main/java/com/example/toyproject/adapter/DataBindingAdapter.kt

+@BindingAdapter("bind:bindImage")
+fun bindImage(imageView: ImageView, imageUri: String) {
+    if (imageUri.isNotEmpty())
+    Glide.with(imageView.context).load(imageUri).into(imageView)


kotlin style guide에 따르면, 실행문이 단문이더라도 괄호로 감싸주는 것이 권장사항입니다.
관련 링크 : https://developer.android.com/kotlin/style-guide/#braces

RecyclerView와 마찬가지로 확잠 하수로 만들면 어떨까요?

혹시 이렇게 바꾸면 말씀하신 확장함수일까요 ? 검색해보니 이런 형태인것 같아서요
8ab4e5e

@handnew04 네 맞습니다 ㅎㅎ

zion830 · 2020-03-13T18:14:06Z

ToyProject/app/src/main/java/com/example/toyproject/data/remote/MovieRemoteDataSourceImpl.kt

+import retrofit2.Response
+
+class MovieRemoteDataSourceImpl() : MovieRemoteDataSource {
+    val APP_KEY = "e29f73abf94a892a99c8df777d038279"


이 클래스 내부에서만 쓰이는 KEY 값이므로 private를 붙여야할거같네요 ㅎㅎ

현재 앱 같은경우는 크게 관계 없겠지만
appKey나 서버주소 같은 내용들은 숨겨지는 편이 좋습니다.
app모듈의 build.gradle에서 BuildConfig내부 변수로 추가하시는 편을 추천 드립니다.

10c37b6
BuildConfig 변수라는 거 처음알았어요ㅜㅜ 감사합니당!!

zion830 · 2020-03-13T18:16:04Z

ToyProject/app/src/main/java/com/example/toyproject/data/entity/MovieResponse.kt

+    val id: String,
+    val poster_path: String
+) {
+    fun getPosterUrl(): String = "https://image.tmdb.org/t/p/w200/$poster_path"


이미지를 부를때 사용하는 이 URL(https://image.tmdb.org/t/p/w200/)이 재사용되거나 수정되는 상황을 고려했을 때, 상수로 따로 분리하면 좋을 것 같아요~

cce7801
감사합니다. 상수를 따로 파일을 만들어서 뒀는데 이렇게 하는게 좋을까요?

zion830 · 2020-03-13T18:18:27Z

ToyProject/app/src/main/java/com/example/toyproject/data/entity/MovieResponse.kt

+
+data class SearchMovieData(
+    val id: String,
+    val poster_path: String


변수 네이밍은 Camel case로 하는 것이 공식 가이드입니다.
json key에 맞춰서 쓰신거라면 모든 프로퍼티에 개별적으로 @SerializedName을 붙이면 될 거 같아요!
관련 링크 : https://developer.android.com/kotlin/style-guide/#naming_2

zion830 · 2020-03-13T18:29:11Z

ToyProject/app/src/main/java/com/example/toyproject/network/RetrofitService.kt

+
+    @GET("search/movie")
+    fun requestSearchMovie(
+        @Query("api_key") api_key: String,


여기도 마찬가지로 camel case로 바꿔야할것 같네용

c04fe6d
감사합니다!

zion830 · 2020-03-13T18:32:05Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainActivity.kt

+
+class MainActivity : AppCompatActivity() {
+    lateinit var binding: ActivityMainBinding
+    lateinit var adapter: MainMoviePostingRecyclerAdapter


adapter는 context가 필요하지 않아서 굳이 lateinit로 선언하지 않아도 될것같네요.
실수로 adapter가 초기화되기 전에 사용하면 NPE가 발생할 수 있기 때문에 선언과 동시에 초기화하는 편이 좋을 것 같습니다 ~

c04fe6d
그 부분은 생각하지 못했는데, 감사합니다!

zion830 · 2020-03-13T18:34:00Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainActivity.kt

+        binding.rcvPoster.adapter = adapter
+    }
+
+    private fun test() {


여기선 test 보다는 초기화, 데이터 로딩, 시작 등의 의미가 느껴지는 네이밍을 사용한다면 역할이 더 잘 와닿을거 같습니다 ㅎㅎ

c04fe6d
수정했습니다. 감사합니다!

zion830 · 2020-03-13T18:35:44Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainActivity.kt

+    private fun initBinding() {
+        binding.vm = mainViewModel
+        binding.lifecycleOwner = this@MainActivity
+        binding.rcvPoster.adapter = adapter


동일한 표현이지만 한 객체의 여러 프로퍼티를 초기화한다면 apply도 사용 가능합니다

binding.apply { viewModel = mainViewModel lifecycleOwner = this@MainActivity rcvPoster.adapter = adapter }

c04fe6d
앗 감사합니다!

zion830 · 2020-03-13T18:38:30Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainMoviePostingRecyclerAdapter.kt

+        return ViewHolder(binding)
+    }
+
+    override fun getItemCount(): Int {


= 로 더 간결하게 표현이 가능합니다!

c04fe6d
감사합니다. 계속 return 을 쓰네요.. ㅠㅠ

dlwls5201 · 2020-03-14T01:12:14Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainViewModel.kt

+            }
+            isLoading.value = true
+        }
+        isLoading.value = false


isLoading.value = true 과 isLoading.value = flase 를 각각 하나의 함수로 만들어서 의미를 명확히 표현하는게 어떨까요?

showLoading(), hideLoading() 등등

3b18433
늦어서 죄송해요 감사합니다.

dlwls5201 · 2020-03-14T01:15:43Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainViewModel.kt

+    val isLoading = MutableLiveData<Boolean>()
+
+    fun searchMovie() {
+        runBlocking {


저도 공부중이지만 안드앱에서 runBlocking 사용은 매우 위험하다고 합니다.
viewModel을 사용하신다면 viewModelScope를 사용해 보시면 좋을 것 같습니다!

55dbfed
감사합니다!

devLibCH

늦은 감이 있지만 코드리뷰 드립니다~
다른분들이 잘 해주셔서 내용이 별로 없네요

devLibCH · 2020-03-14T14:22:39Z

ToyProject/app/src/main/java/com/example/toyproject/data/remote/MovieRemoteDataSourceImpl.kt

+import retrofit2.Response
+
+class MovieRemoteDataSourceImpl() : MovieRemoteDataSource {
+    val APP_KEY = "e29f73abf94a892a99c8df777d038279"


현재 앱 같은경우는 크게 관계 없겠지만
appKey나 서버주소 같은 내용들은 숨겨지는 편이 좋습니다.
app모듈의 build.gradle에서 BuildConfig내부 변수로 추가하시는 편을 추천 드립니다.

devLibCH · 2020-03-14T14:29:35Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainViewModel.kt

+    val query = MutableLiveData<String>()
+
+    fun searchMovie() {
+        MovieRepositoryImpl.getMovieData(query.toString(), success = {


liveData를 사용하셨으므로 query.value를 사용하는 것 이 좋을 것 같네요

3b18433
늦어서 죄송해요. 감사합니다 ! ㅜㅜ

devLibCH · 2020-03-14T14:49:53Z

ToyProject/app/src/main/java/com/example/toyproject/ui/main/MainMoviePostingRecyclerAdapter.kt

+
+    inner class ViewHolder(val binding: ItemMainBinding) : RecyclerView.ViewHolder(binding.root) {
+        init {
+            binding.root.setOnClickListener({


dataBinding을 사용하시니까 xml에서 직접 clickListener를 사용하는 것이 어떨까요?

3b18433
감사합니다 ㅜㅜ!

devLibCH · 2020-03-14T15:09:47Z

ToyProject/app/src/main/res/layout/activity_main.xml

+            app:layout_constraintEnd_toEndOf="parent"
+            app:layout_constraintStart_toStartOf="parent"
+            app:layout_constraintTop_toTopOf="parent"
+            android:visibility="@{vm.isLoading() ? View.VISIBLE : View.GONE}"


extension함수를 만들어서 사용하는게 좋을 것 같습니다

[질문] VISIBLE, GONE 중 하나만 선택하면 되는 간단한 분기라 custom attribute가 필요해보이지 않는데, 확장 함수를 사용하면 어떤 장점이 있는지 궁금합니당

[질문] VISIBLE, GONE 중 하나만 선택하면 되는 간단한 분기라 custom attribute가 필요해보이지 않는데, 확장 함수를 사용하면 어떤 장점이 있는지 궁금합니당

개인적으로는

View를 import 하지 않아도 된다.

View.{함수} 형식으로 모든 뷰에서 일괄적으로 관리할 수 있다.

더 있을까요 ?_?

bc145e2
감사합니당!

handnew04 added 10 commits February 23, 2020 02:02

프로젝트 정리

bfa8270

gradle 수정, mainActivity 패키지 수정

f7e7548

code style 변경, layout recyclerView 추가

3a1810a

retrofit 검색 api 연결 생성

34e7087

MovieResponse 수정, RemoteDataSource 추가

e174844

Repository 추가. Retrofit Helper, Service 추가

46a946a

viewModel 추가, layout DataBinding 적용, DataBindingAdapter 추가

8ddd99b

MainActivity 수정, rcvAdapter 추가

15754e2

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

ccc2a2b

coroutine 사용

c37aaab

handnew04 self-assigned this Mar 8, 2020

handnew04 requested review from devLibCH and dlwls5201 March 8, 2020 15:23

zion830 requested changes Mar 13, 2020

View reviewed changes

dlwls5201 reviewed Mar 14, 2020

View reviewed changes

devLibCH requested changes Mar 14, 2020

View reviewed changes

handnew04 added 7 commits March 16, 2020 21:35

DataBindinAdapter 수정

8ab4e5e

appkey buildConfig 변수로 변경

10c37b6

image path 상수로 변경, initLogInterceptor 변수 수정

cce7801

변수 및 메서드 명 변경, adapter 초기화 시점 변경, 코드 일부분 간결한 형태로 변경

c04fe6d

isLoading 처리부분 함수로 변경, 아이템 클릭리스너 호출 xml 로 변경, toString -> value 로 변경

3b18433

progressbar visibility extension 함수로 변경

bc145e2

runblocking 을 viewmodelScope 로 변경

55dbfed

영화 목록 출력 #22

Are you sure you want to change the base?

영화 목록 출력 #22

Conversation

handnew04 commented Mar 8, 2020

zion830 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlwls5201 Mar 14, 2020 • edited Loading

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

devLibCH 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

Choose a reason for hiding this comment

dlwls5201 Mar 14, 2020 •

edited

Loading