S song develop #12

SSong-develop · 2020-11-20T09:27:23Z

No description provided.

jinsu4755

일단 미리 보라고 리뷰 하나 남깁니다 나머지는 바로 해볼께

jinsu4755 · 2020-11-20T11:25:13Z

app/src/main/java/com/siba/searchmvvmpractice/ui/base/SearchViewModelFactory.kt

+        if(modelClass.isAssignableFrom(SearchViewModel::class.java)){
+            return SearchViewModel(repository) as T
+        }
+        throw IllegalArgumentException("Unknown class name")


Suggested change

if(modelClass.isAssignableFrom(SearchViewModel::class.java)){

return SearchViewModel(repository) as T

}

throw IllegalArgumentException("Unknown class name")

require(modelClass.isAssignableFrom(SearchViewModel::class.java)) { "Unknown class name" }

return SearchViewModel(repository) as T

동일 코드인데 if문을 조금더 줄여서 사용이 가능합니다! 이런 방법도 있구나 알고 있으면 좋음!

l2hyunwoo

형님 수고하셨습니다.

l2hyunwoo · 2020-11-20T10:19:21Z

app/src/main/java/com/siba/searchmvvmpractice/ui/adapter/RepoAdapter.kt

+    inner class RepoViewHolder<B : RepoItemBinding>(itemView : View) : RecyclerView.ViewHolder(itemView) {
+        val binding : B = DataBindingUtil.bind(itemView)!!
+        fun bind(userRepository : UserRepository){
+            binding.setVariable(BR.userRepository,userRepository)
+        }
+    }


Suggested change

inner class RepoViewHolder<B : RepoItemBinding>(itemView : View) : RecyclerView.ViewHolder(itemView) {

val binding : B = DataBindingUtil.bind(itemView)!!

fun bind(userRepository : UserRepository){

binding.setVariable(BR.userRepository,userRepository)

}

}

inner class RepoViewHolde(private val binding: ViewDataBinding) : RecyclerView.ViewHolder(binding.root) {

fun bind(userRepository : UserRepository){

binding.setVariable(BR.userRepository,userRepository)

}

}

역시천재

l2hyunwoo · 2020-11-20T11:20:04Z

app/src/main/java/com/siba/searchmvvmpractice/ui/adapter/SearchTermAdapter.kt

+
+    override fun getItemCount(): Int = data.size
+
+    internal fun setData(recentSearchTerm: List<RecentSearchTerm>){


결국엔 이것도 public이네....(같은 모듈에서만 사용하는거니까) 그냥 접근제한자 없애도 될 것 같아

l2hyunwoo · 2020-11-20T11:20:19Z

app/src/main/java/com/siba/searchmvvmpractice/ui/adapter/SearchTermAdapter.kt

+    inner class SearchTermViewHolder<B : SearchTermItemBinding>(itemView : View) : RecyclerView.ViewHolder(itemView) {
+        val binding : B = DataBindingUtil.bind(itemView)!!
+        fun bind(recentSearchTerm: RecentSearchTerm){
+            binding.setVariable(BR.searchTerm,recentSearchTerm)
+        }
+    }


카톡에 있는거 참고

l2hyunwoo · 2020-11-20T11:21:30Z

app/src/main/res/layout/repo_item.xml

+            android:id="@+id/login"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            android:layout_marginLeft="16dp"


Suggested change

android:layout_marginLeft="16dp"

android:layout_marginStart="16dp"

l2hyunwoo · 2020-11-20T11:21:48Z

app/src/main/res/layout/repo_item.xml

+        android:layout_width="match_parent"
+        android:layout_height="wrap_content">
+        <TextView
+            android:id="@+id/login"


id 이름이 뭐하는 건지 모르겠군요

l2hyunwoo · 2020-11-20T11:22:06Z

app/src/main/res/layout/repo_item.xml

+            android:id="@+id/url"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            android:layout_marginTop="10dp"


4의 배수 맞추시고

l2hyunwoo · 2020-11-20T11:22:21Z

app/src/main/res/layout/repo_item.xml

+            app:layout_constraintTop_toTopOf="parent" />
+
+        <TextView
+            android:id="@+id/url"


id 쌉극혐이고

l2hyunwoo · 2020-11-20T11:23:38Z

app/src/main/res/layout/user_item.xml

+    </data>
+
+    <androidx.constraintlayout.widget.ConstraintLayout
+        android:layout_width="401dp"


401dp 모냐고~

l2hyunwoo · 2020-11-20T11:24:25Z

app/src/main/res/layout/user_item.xml

+        <TextView
+            android:id="@+id/login"
+            android:layout_width="wrap_content"
+            android:layout_height="wrap_content"
+            android:layout_marginLeft="16dp"
+            android:layout_marginTop="16dp"
+            android:text="@{users.login}"
+            android:textSize="12sp"
+            app:layout_constraintStart_toStartOf="parent"
+            app:layout_constraintTop_toTopOf="parent" />
+
+        <TextView
+            android:id="@+id/url"


다른 뷰인데 같은 id가 보이네...나중에 헷갈릴 수 있을 것 같아

l2hyunwoo · 2020-11-20T11:25:14Z

app/src/main/res/layout/fragment_search_user.xml

+    </data>
+
+    <androidx.constraintlayout.widget.ConstraintLayout
+        android:id="@+id/frameLayout2"


호오 쒸잇

jinsu4755 · 2020-11-20T11:38:03Z

불필요한 제네렉 삭제해주세요~~~~

jinsu4755 · 2020-12-02T14:15:10Z

app/src/main/java/com/siba/searchmvvmpractice/ui/adapter/UserAdapter.kt

+import com.siba.searchmvvmpractice.remote.model.Users
+
+class UserAdapter<B : UserItemBinding> : RecyclerView.Adapter<UserAdapter<B>.UserViewHolder<B>>() {
+    var data = mutableListOf<Users>()


Adapter를 제작할때 내부에 데이터들은 밖으로 줄 필요가 없으니 불필요한 getter를 만드는 var은 별로 좋지 않은거 같아.
val로 불변하게 만들어서 clear같은걸 이용해보는건 어때?
아니면 해당 어뎁터를 일급 컬랙션으로 래핑해서
어떤 메소드를 통해 데이터를 변경하라는 메시지를 던지는게 더 좋을 거 같아

jinsu4755

의견 남겨봤어~ 한번 보고 다시 의견 남겨줘도 좋고 굿잡 형이 젤 진도 빠르네 멋지다..

jinsu4755 · 2020-12-02T17:06:18Z

app/src/main/java/com/siba/searchmvvmpractice/local/database/SearchTermDatabase.kt

+        fun getInstance(context: Context): SearchTermDatabase {
+            synchronized(this) {
+                var instance = INSTANCE
+
+                if (instance == null) {
+                    instance = Room.databaseBuilder(
+                        context.applicationContext,
+                        SearchTermDatabase::class.java,
+                        "search_keyword_history_database2"
+                    ).build()
+                    INSTANCE = instance
+                }
+                return instance


이 부분에 대한 코드인데 사실 구글에서 하라는 대로긴 하지만 너무 깊게 depth가 들어가는 부분이라 나는 이 부분은 좀 자바 같다고 생각하는데 어떻게 생각해?
depth를 줄이고 가독성 있게 만드는 방법이 있지 않을까?

그리고 약간 return 값이 instance인데 내부 변수인 이 친구를 던지는 이유는 뭘까..? 따로 변수를 만들지 않아도 충분히 가능한 부분 같아

Suggested change

fun getInstance(context: Context): SearchTermDatabase {

synchronized(this) {

var instance = INSTANCE

if (instance == null) {

instance = Room.databaseBuilder(

context.applicationContext,

SearchTermDatabase::class.java,

"search_keyword_history_database2"

).build()

INSTANCE = instance

}

return instance

fun getInstance(context: Context): SearchTermDatabase = INSTANCE ?: synchronized(this) {

INSTANCE ?: Room.databaseBuilder(

context.applicationContext,

SearchTermDatabase::class.java,

"search_keyword_history_database2"

).build().apply {

INSTANCE = this

}

}

이런 식으로 하면 기존 코드보다 뎁스도 줄이고 가독성도 해치지 않는다고 생각해

해당 함수가 SearchTermDatabase 타입의 INSTANCE를 던져주는데 만약 null이라면 동기화 블럭에 들어가서 Room 데이터베이스 빌더로 데이터 베이스 만들어주고 그걸 INSTANCE에 apply하는거지

기존 코드랑 똑같은데 다르게 작성해서 indent 를 줄이고 조금더 읽으면서 한눈에 파악되게 작성한거고

큰 차이점이 있다면 미리 우리가 선언해둔 인스턴스에 값이 들어있다면 동기화 블럭을 거치지 않아도 해당 인스턴스를 던져주는 느낌?
난 기존코드를 생각했을때 INSTANCE가 있던 없던 동기화 블럭에 간다고 생각해서
굳이 이미 INSTANCE가 있을때도 멀티스레드 세이프티하게 인스턴스를 던져줄 필요가 없다고 생각했어 어짜피 그땐 동일한 INSTANCE를 가져가니까

jinsu4755 · 2020-12-02T18:21:09Z

app/src/main/java/com/siba/searchmvvmpractice/remote/RetrofitService.kt

+import retrofit2.http.GET
+import retrofit2.http.Query
+
+interface RetrofitService {


리트로핏 서비스 리트로핏에 대한 서비스? 뭔가 이름이 이상하다고 생각해
나였으면 GitHubSearchService 혹은 SearchService 로 할 것 같아

jinsu4755 · 2020-12-02T18:22:50Z

app/src/main/java/com/siba/searchmvvmpractice/remote/RetrofitService.kt

+    ): UserCatalog
+
+    @GET("search/repositories")
+    suspend fun getRepositories(


이 프로젝트 안에서도 레포지터리가 있고 우리가 원하는 깃헙의 온라인 레포지터리도 있어서 그 두가지의 구분으로 나라면 getOnlineRepositories 뭐 이런 느낌으로 했을 것 같음!

jinsu4755 · 2020-12-02T18:29:04Z

app/src/main/java/com/siba/searchmvvmpractice/ui/adapter/RepoAdapter.kt

+import com.siba.searchmvvmpractice.databinding.RepoItemBinding
+import com.siba.searchmvvmpractice.remote.model.UserRepository
+
+class RepoAdapter<B : RepoItemBinding> : RecyclerView.Adapter<RepoAdapter<B>.RepoViewHolder<B>>() {


제네릭을 써서 컴파일시 강한 타입 검사와 손쉬운 타입 변화가 될 수 있는 건 좋은데 어짜피 해당 뷰 홀더에는 하나의 뷰 홀더를 사용하고 있고
굳이 Fragment에서 뷰홀더가 써야하는 RepoItemBinding을 엑티비티에서 이런 타입이다 선언해줄 필요가 있을까..? 이미 레포지터리를 위한 어뎁터인데 당연하게 안에 들어가는 바인딩은 레포지터리 아이템에 대한 바인딩이겠지..?

그래서 뭔가 RepoItemBinding 라는 클래스 타입을 B라는 제네릭으로 축약해버린거 같아서 아쉽다...

jinsu4755 · 2020-12-02T18:32:37Z

app/src/main/java/com/siba/searchmvvmpractice/ui/base/SearchViewModelFactory.kt

+        require(modelClass.isAssignableFrom(SearchViewModel::class.java)){"Unknown class name"}
+        return SearchViewModel(repository) as T


굳 코드가 간결하고 더 읽기 쉬워졌네 👍

jinsu4755 · 2020-12-02T18:48:33Z

app/src/main/java/com/siba/searchmvvmpractice/ui/presentation/fragment/SearchRepoFragment.kt

+
+    private fun setObserver() {
+        viewModel.githubRepo.observe(viewLifecycleOwner) {
+            repoAdapter.data = it.userRepository as MutableList<UserRepository>


이부분도 뭔가 어뎁터에 MutableList를 잘 사용하지 못한거 같아. 추가와 삭제가 자유로운데 결국 새로운 변하는 리스트를 대입해버리면 애초에 그냥 repoAdapter.data가 List여도 상관 없는거 아닐까..?

SSong-develop added 22 commits November 11, 2020 18:25

feature : base code setting

1be855a

TODO : How to handle Path value in repos :)

74815ed

feature #1 : UserSearch done :)

56934c2

TODO #1

0416b55

feature #2 : get All User from Github done :)

d33aadc

TODO : TODO Something

82cf42a

feature#2 : get All User done :)

6d9ff13

feature#3 : second Search done :)

cc44c50

[UPDATE] : Regroup Package

2e109e7

[FEATURE] : Room create ;)

8b9c5f3

[FEATURE] : Search Repo done :)

fe5da29

[TODO] UPDATE :)

75b9cb1

[FEATURE] RecentSearchTerm Done :)

08127fc

[FEATURE] 각가 검색기능 done :)

3440b9f

[REFACT] Package Refactor

97cef74

[REFACT] delete not using api

207b6cc

[ERROR] Can't Retrofit

f97a0c2

:(

5e0a31b

[FIX] Data class rename done :)

63d7b11

[FIX] Repo Search Error Fix :)

d128558

[TODO] Todo issue 주석 적어놈

d5d77f6

[UPDATE] Update

f79a554

SSong-develop added the 훈기 훈기 작업 label Nov 20, 2020

SSong-develop requested review from jinsu4755, l2hyunwoo and kym1924 November 20, 2020 09:27

SSong-develop self-assigned this Nov 20, 2020

SSong-develop closed this Nov 20, 2020

SSong-develop reopened this Nov 20, 2020

jinsu4755 reviewed Nov 20, 2020

View reviewed changes

l2hyunwoo suggested changes Nov 20, 2020

View reviewed changes

SSong-develop added 2 commits November 21, 2020 20:54

Optimized Package

130d577

update advice

52d84ba

jinsu4755 reviewed Dec 2, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S song develop #12

S song develop #12

SSong-develop commented Nov 20, 2020

jinsu4755 left a comment

jinsu4755 Nov 20, 2020

jinsu4755 Nov 20, 2020

kym1924 Nov 24, 2020

l2hyunwoo left a comment

l2hyunwoo Nov 20, 2020

kym1924 Nov 24, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

l2hyunwoo Nov 20, 2020

jinsu4755 commented Nov 20, 2020

jinsu4755 Dec 2, 2020

jinsu4755 left a comment

jinsu4755 Dec 2, 2020

jinsu4755 Dec 2, 2020 •

edited

Loading

jinsu4755 Dec 2, 2020 •

edited

Loading

jinsu4755 Dec 2, 2020

jinsu4755 Dec 2, 2020

jinsu4755 Dec 2, 2020

jinsu4755 Dec 2, 2020

jinsu4755 Dec 2, 2020


		override fun getItemCount(): Int = data.size

		internal fun setData(recentSearchTerm: List<RecentSearchTerm>){

	android:layout_marginLeft="16dp"
	android:layout_marginStart="16dp"

		require(modelClass.isAssignableFrom(SearchViewModel::class.java)){"Unknown class name"}
		return SearchViewModel(repository) as T

S song develop #12

Are you sure you want to change the base?

S song develop #12

Conversation

SSong-develop commented Nov 20, 2020

jinsu4755 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

l2hyunwoo 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

jinsu4755 commented Nov 20, 2020

Choose a reason for hiding this comment

jinsu4755 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinsu4755 Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

jinsu4755 Dec 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinsu4755 Dec 2, 2020 •

edited

Loading

jinsu4755 Dec 2, 2020 •

edited

Loading