코인 앱 만들기 by sport0102 · Pull Request #3 · SkyGarden3/RxJavaStudy

sport0102 · 2019-12-19T16:40:12Z

No description provided.

preludezdev

안녕하세요 선민님,
부족하지만 열심히 리뷰 해봤습니다.
저도 모르는 부분이 많아 나머지 궁금한점은 스터디 때 같이 얘기해보면 좋을 것 같아요 :D

preludezdev

안녕하세요 선민님
부족하지만... 일단 보면서 바로 보이는 것들만 리뷰 달아보았습니다
나머지는 우석님께 맡길게요.
그럼 내일 뵐게요~~
(내일 코루틴 세미나 기대할게요 :D)

preludezdev · 2020-02-08T01:58:35Z

+    /**
+     * Returns the content, even if it's already been handled.
+     */
+    fun peekContent(): T = content
+}


Event 클래스 아래에

/** * An [Observer] for [Event]s, simplifying the pattern of checking if the [Event]'s content has * already been handled. * * [onEventUnhandledContent] is *only* called if the [Event]'s contents has not been handled. */ class EventObserver<T>(private val onEventUnhandledContent: (T) -> Unit) : Observer<Event<T>> { override fun onChanged(event: Event<T>?) { event?.getContentIfNotHandled()?.let { onEventUnhandledContent(it) } } }

추가하시면 데이터 옵저빙할 때 getContentIfNotHandled() 안쓰고 더 간편하게 코드 작성하실 수 있을거에요 :)

42a1d75
완료

preludezdev · 2020-02-08T01:59:26Z

+package com.study.myapplication.api
+
+import com.study.myapplication.api.model.BithumbTickerResponse
+import retrofit2.http.GET
+
+interface BithumbApi {
+
+    @GET("public/ticker/all_krw")
+    suspend fun getTickerInfo(): BithumbTickerResponse
+}


api 패키지는 데이터 레이어 안으로 넣으셔도 될거같아요 :)

03e4616
완료

preludezdev · 2020-02-08T02:00:05Z

+package com.study.myapplication.base
+
+import android.os.Bundle
+import android.widget.Toast
+import androidx.annotation.LayoutRes
+import androidx.appcompat.app.AppCompatActivity
+import androidx.databinding.DataBindingUtil
+import androidx.databinding.ViewDataBinding
+import androidx.lifecycle.ViewModel
+import com.study.myapplication.BR
+
+
+abstract class BaseActivity<B : ViewDataBinding, VM : ViewModel>(


마찬가지로 Base 패키지는 프레젠테이션 안으로 넣어주셔도 좋을거 같아요

9b41bf2
완료

preludezdev · 2020-02-08T02:01:30Z

+package com.study.myapplication.di
+
+import com.study.myapplication.feature.compare.CompareCoinViewModel
+import org.koin.androidx.viewmodel.dsl.viewModel
+import org.koin.dsl.module
+
+fun getAppModule() = module {
+    viewModel {
+        CompareCoinViewModel(
+            get(),
+            get(),
+            get(),
+            get()
+        )
+    }
+}


지금은 레이어 분리를 모듈단위가 아니고 패키지 단위로 하고 있어서 di 가 하나에 다 있는데 공부하는 거니까 패키지로 분리하더라도 di 는 각 레이어 패키지마다 있으면 좋을 것 같습니다

b92296b
완료

preludezdev · 2020-02-08T02:02:24Z

+package com.study.myapplication.ext
+
+import android.widget.ImageView
+import androidx.databinding.BindingAdapter
+import com.bumptech.glide.Glide
+
+
+@BindingAdapter("bind:imageUrl")
+fun ImageView.bindImageUrl(imageUrl: String?) {
+    imageUrl?.let {
+        Glide.with(this.context)
+            .load(it)
+            .into(this)
+    }
+}


마찬가지로 ext 패키지도 프레젠테이션 레이어로 고고

ae80fce
완료

preludezdev · 2020-02-08T02:04:06Z

+package com.study.myapplication.feature.compare
+
+import android.os.Bundle
+import androidx.lifecycle.Observer
+import com.study.myapplication.R
+import com.study.myapplication.base.BaseActivity
+import com.study.myapplication.databinding.ActivityCompareCoinBinding
+import org.koin.androidx.viewmodel.ext.android.viewModel
+
+class CompareCoinActivity :
+    BaseActivity<ActivityCompareCoinBinding, CompareCoinViewModel>(R.layout.activity_compare_coin) {


이 패키지는 프레젠테이션 레이어가 되면 될거같아요

435468b
완료

preludezdev · 2020-02-08T02:05:01Z

+package com.study.myapplication.utils
+
+import kotlin.math.abs
+
+object StringUtil {
+


모듈단위로 분리했다고 가정하면 유틸을 모아둔 패키지도 각 레이어마다 있게 될거같아요

b598a32
완료

preludezdev · 2020-02-08T02:12:08Z

+package com.study.myapplication.data.source
+
+import com.study.myapplication.domain.entity.Market
+import com.study.myapplication.domain.entity.Ticker
+
+interface CoinRepository {
+
+    suspend fun getUpbitMarket(): List<Market>
+
+    suspend fun getUpbitCoin(marketList: List<Market>): Map<String, Ticker>
+
+    suspend fun getBithumbCoin(): Map<String, Ticker>
+
+    suspend fun getCoinOneCoin(): Map<String, Ticker>
+
+}


레포지토리 인터페이스는 도메인 레이어에 있어야됩니당
(지금은 자기자신만 알아야할 도메인이 데이터 레이어에 있는 인터페이스를 알고있는 구조에요)

9470fe5
완료

GongSulMin(공설민) 공간입니다.

wswon · 2020-02-08T18:26:44Z

+        setupKoin(
+            this,
+            getNetworkModule(
+                "https://api.upbit.com",


상수나 string resource로 사용하면 좀 더 좋지 않을까요?

ad79f3d
완료

wswon · 2020-02-08T18:29:22Z

+        binding.run(action)
+    }
+
+    fun toastM(message: String) {


Suggested change

fun toastM(message: String) {

protected fun toastM(message: String) {

상속하는 activity에서만 접근하는 함수라면 이렇게 제한해주면 좀 더 좋지않을까 생각해봤습니다!

1b06fb5
완료

wswon · 2020-02-08T18:36:36Z

+    single(named("upbitApi")) {
+        Retrofit.Builder()
+            .client(get())
+            .addConverterFactory(get())
+            .baseUrl(upbitUrl)
+            .build()
+            .create(UpbitApi::class.java)
+    }


single<UpbitApi> { Retrofit.Builder() .client(get()) .addConverterFactory(get()) .baseUrl(upbitUrl) .build() .create(UpbitApi::class.java) }

매번 name으로 접근하는 것 보다는 타입을 지정해서 접근한다면,
get할때 name을 넣어주지 않아도 되도록 만들 수 있지 않을까 생각합니다.

Retrofit도 잘라서 만들 것

05eebd8
완료

wswon · 2020-02-08T18:41:31Z

+package com.study.myapplication.feature.compare.model
+
+data class CompareCoinInfo(
+    var coinName: String? = null,


Suggested change

var coinName: String? = null,

val coinName: String = "",

not null type을 사용하는게 좀 더 좋지 않을까요?

6861f30
완료

wswon · 2020-02-08T18:49:28Z

+            val upbitTickerList = try {
+                upbitTickerList.await()
+            } catch (e: Exception) {
+                Log.d("CompareCoinViewModel", "${e.message}")
+                _isDataLoadingError.value = Event("업비트 데이터 로딩 에러 발생" to true)
+                _isDataLoading.value = false
+                mapOf<String, Ticker>()
+            }


Exception에 대한 handling을 try ~ catch 를 통해 viewmodel 에서 보다는 model에서 하고,
결과를 내려받는 것이 역할에 맞는것이 아닐까? 하고 생각해봤습니다.
viewmodel 에서 데이터를 로딩하는 것에 대한 판단을 하고있는 것으로 보여서요

repository에 있으면 좋겠다

Result 클래스로 리턴을 받아라

entity에는 id를 넣어라 UUID.Rand~~ 함수를 하면된다

이거는 좀 적용하는데 시간이 걸릴 것 같습니다 추후에 적용해서 다시 올리겠습니다

wswon · 2020-02-08T18:53:04Z

+            _coinList.postValue(
+                getCompareCoinList(
+                    upbitTickerList,
+                    bithumbTickerList,
+                    coinOneTickerList
+                )
+            )
+            _isDataLoading.value = false


이부분에서 아래의 _isDataLoading.value 에서는 postValue가 아닌 value를 사용하고 있는데
그렇다면 위의 _coinList도 value로 처리해도 되지않을까요?

5eeaa7f
완료

wswon

부족하지만 리뷰 남겨보았습니다 🤗

wswon

추가된 커밋을 일일히 확인하였는데,
저는 더 리뷰할 부분이 없네요. 고생하셨습니다!! 👍

sport0102 added 8 commits December 20, 2019 01:23

init commit

2f551f6

di 추가

17e5717

base 추가

a2aa1a9

API 콜 추가

731d7c7

model 관련 추가

d97f137

ui 추가

2f25cc5

extension 추가

948530e

검색 로직 추가

7a34471

sport0102 added the Review Needed 리뷰어의 리뷰가 필요한 경우 label Dec 19, 2019

sport0102 requested a review from preludezdev December 19, 2019 16:40

sport0102 assigned preludezdev and sport0102 and unassigned preludezdev Dec 19, 2019

sport0102 changed the title ~~리뷰 부탁드립니당!~~ HW-1 RxJava 로 네이버 영화검색 앱 만들기 Dec 20, 2019

recyclerview item 클릭 시 해당 링크로 이동

3c78253

preludezdev reviewed Dec 20, 2019

View reviewed changes

변수명 수정

fe141e2

sport0102 requested a review from preludezdev December 21, 2019 14:12

sport0102 added 12 commits December 22, 2019 16:24

API 키 전역으로 처리할 수 있게 변경

d4cf4f4

LiveData configuration change 시 데이터 한번 더 발행 되는 행위 Event 객체로 처리하게 수정

3f97116

LiveData configuration change 시 데이터 한번 더 발행 되는 행위 Event 객체로 처리하게 수정

aa3e0d6

검색 후에 검색어 지우는 로직 삭제

30487d4

activity에서 disposable 처리

9af1d83

이미지에 radius 처리

916457b

elevation 처리 삭제

312adee

Naver 관련 소스 삭제

8545892

오타 수정

55201f7

resource 추가

4e3816d

네이버 관련 코드 삭제

d07b902

Util 추가

73ace28

sport0102 added 4 commits January 26, 2020 18:07

coroutines 라이브러리 추가

937bef1

library 추가 및 삭제

fc31a05

코루틴으로 API 가져오게 변경

43cd4c6

클린 아키텍쳐 entity 적용, loading 창 적용, 코루틴 에러처리 적용

b247480

sport0102 changed the title ~~HW-1 RxJava 로 네이버 영화검색 앱 만들기~~ 코인 앱 만들기 Feb 8, 2020

preludezdev reviewed Feb 8, 2020

View reviewed changes

NackUn pushed a commit to NackUn/RxJavaStudy that referenced this pull request Feb 8, 2020

Merge pull request SkyGarden3#3 from GongSulMin/master

1ba6adc

GongSulMin(공설민) 공간입니다.

wswon self-requested a review February 8, 2020 18:23

wswon reviewed Feb 8, 2020

View reviewed changes

wswon added Answer Needed PR담당자의 응답이 필요한 경우 and removed Review Needed 리뷰어의 리뷰가 필요한 경우 labels Feb 8, 2020

sport0102 added 13 commits February 9, 2020 14:14

Repository interface 패키지 이동(data to domain)

9470fe5

Event 클래스 패키지 이동 및 EventObserver 생성, 적용

42a1d75

api 패키지 data 패키지로 이동

03e4616

feature 패키지 이름 presentation으로 수정

435468b

base 패키지 presentation 패키지로 이동

9b41bf2

멀티 모듈 고려하여 각 layer 별로 di 패키지 생성 및 이동

b92296b

Ext 패키지 사용하는 layer의 하위로 이동

ae80fce

utils 패키지 사용 layer 하위로 이동

b598a32

하드코딩 대신 변수로 Url 관리하게 수정

ad79f3d

접근 제한자 추가

1b06fb5

Koin 반환 타입 지정 및 Retrofit 단위 분리

05eebd8

livedata set value하는 부분 수정

5eeaa7f

non-null 하게 수정

6861f30

sport0102 requested review from preludezdev and wswon February 9, 2020 07:02

sport0102 added Review Needed 리뷰어의 리뷰가 필요한 경우 and removed Answer Needed PR담당자의 응답이 필요한 경우 labels Feb 9, 2020

wswon approved these changes Feb 11, 2020

View reviewed changes

	fun toastM(message: String) {
	protected fun toastM(message: String) {

Conversation

sport0102 commented Dec 19, 2019

Uh oh!

preludezdev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

preludezdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sport0102 Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preludezdev Feb 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sport0102 Feb 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sport0102 Feb 9, 2020 •

edited

Loading

preludezdev Feb 8, 2020 •

edited

Loading

sport0102 Feb 9, 2020 •

edited

Loading