[부나] 3, 4단계 쇼핑 장바구니 제출합니다. #45
Conversation
|
컨플릭이 너무 많이 발생하고 있어서 리뷰가 어렵네요 ㅠ_ㅠ 먼저 컨플릭 해결 후에 다시 리뷰 요청 부탁드릴게요 🙂 |
tmdgh1592
force-pushed
the
step4
branch
2 times, most recently
from
345dafb to
d52a407
Compare
- previous, next -> previousEnabled, nextEnabled - getPartially(), add() -> getRecentProductsPartially(), addRecentProduct()
| shoppingService.start() | ||
| shoppingService.join() |
There was a problem hiding this comment.
ShoppingMockWebServer 는 Thread 인가요? 적절한 상속관계인지 고민해보셨으면 좋겠어요 :)
생성자에서 반드시 start() join()을 호출해주어야한다는 사실을 다른 개발자들이 알 수 있을까요?
RemoteProductDataSource는 테스트할 수 있는 구조일까요?
Thread를 실행시켜야만 내부 mockWebServer 객체 내부가 초기화되는데, 이러한 지연 로드를 해야하는 이유를 이 구조에서 찾기가 어려워보여요.
쓰레드를 실행시키지 않고 MockWebServer내 정보를 초기화 시키면, 특정 문제가 발생했었나요? 🤔
| override fun getProductByPage(page: Page): List<Product> { | ||
| shoppingService.join() | ||
| val url = "${BASE_URL}/products?start=${page.start}&count=${page.sizePerPage}" | ||
| val httpClient = OkHttpClient() | ||
| val request = Request.Builder().url(url).method(GET, null).build() | ||
| val products = mutableListOf<Product>() | ||
| val countDownLatch = CountDownLatch(1) |
There was a problem hiding this comment.
이 메서드에서 CountDownLatch를 await 시키면, 어떤 쓰레드가 블락될까요?
어떤 현상들이 생길 수 있을까요?
There was a problem hiding this comment.
Main Thread가 block 된다고 생각합니다.
만약 계속해서 block된다면 심각한 경우 ANR로 이어질 수 있을 듯 합니다..!
| httpClient.newCall(request).enqueue(object : Callback { | ||
| override fun onFailure(call: Call, e: IOException) { | ||
| countDownLatch.countDown() | ||
| } |
There was a problem hiding this comment.
각각 동기 / 비동기 차이라고 알고 있습니다!
비동기로 구현하려고 했는데, 사실상 동기적으로 구현해버렸네요..!
현재는 callback으로 변경하였습니다!
|
|
||
| abstract class Presenter(protected val view: View) { |
There was a problem hiding this comment.
Presenter만 abstract로 선언한 이유가 무엇인가요?
View를 반드시 생성자로 받아야만 하는 이유가 궁금해요 :)
There was a problem hiding this comment.
view와 presenter가 1:1 관계라고 생각해서 abstract로 선언하였습니다.
크게 차이가 없다고 생각하였는데, 지금 interface로 바꿔보니 View와 Presenter간의 Contract를 더 보기 편해서,
모두 interface로 변경하였습니다!
| view.updateNavigatorEnabled(currentPage.hasPrevious(), currentPage.hasNext(cart)) | ||
| view.updatePageNumber(currentPage.toUi()) |
There was a problem hiding this comment.
하나의 행위를 하기 위해 view를 여러 번 부르고 있네요.
갱신해야할 View가 n개가 된다면 이 로직도 n줄이 되어야할까요?
view를 갱신하는 로직은 한 번만 호출하되, View에서 분리된 메서드를 호출하는 형태는 어떨까요?
There was a problem hiding this comment.
View의 추상 메서드를 하나로 줄이고 Activity에서 별도로 분리해서 사용하도록 변경하였습니다!
| fun inject(): ProductDataSource.Remote = | ||
| RemoteProductDataSource() | ||
|
|
||
| fun inject(dao: RecentProductDao): RecentProductDataSource.Local = | ||
| LocalRecentProductDataSource(dao) | ||
|
|
||
| fun inject(dao: CartDao): CartDataSource.Local = | ||
| LocalCartDataSource(dao) |
There was a problem hiding this comment.
메서드명만 보고 어떤 객체가 생성될 지 누구나 예측할 수 있을까요?
인터페이스는 같지만 다른 다형성을 지닌 객체를 받고 싶은 경우 손쉽게 넘겨줄 수 있는 구조인지 고민해보세요 :)
There was a problem hiding this comment.
마치 DI 라이브러리처럼 구현해보고 싶어서 이렇게 해보았습니다.
하지만 말리빈 말씀처럼 다형성에 대한 처리가 힘들고, inject만으로 되어 있다보니, 가독성 / 유지보수가 너무 떨어진다는 것을 다음 미션을 하면서 느꼈습니다 🥲
현재는 조금 더 명시적으로 네이밍을 변경하였습니다!
There was a problem hiding this comment.
Koin 라이브러리가 비슷한 형태로 되어있지요.
어디서든 koin 스코프 내에서 get()을 호출하면, 메모리 트리에서 적절한 인스턴스를 찾아내어서 원하는 인스턴스를 가져오게 만들지요.
하지만 get()을 부르기 위해 사용자가 직접 get 메서드를 구현할 필요는 없습니다.
이런 것처럼 만들어보려 했던 걸로 이해했어요!
DI 구현은 굉장히 고려해야할 게 많다고 생각이 들어서... ㅋㅋㅋ 라이브러리 들여다보는 정도만 해도 충분하지 않을까~ 하는 생각이 있어요 :)
물론 구현해보시고 싶다면, 말리진 않겠읍니다 💪
| inject(inject()), | ||
| inject(inject(injectRecentProductDao(database))), | ||
| inject(inject(injectCartDao(database))), | ||
| ) |
| verify(exactly = 1) { view.updateCart(any()) } | ||
| verify(exactly = 1) { view.updateNavigatorEnabled(any(), any()) } | ||
| verify(exactly = 1) { view.updatePageNumber(any()) } |
| val currentPage = slot<Page>() | ||
|
|
||
| // when | ||
| presenter.fetchCart(page + 1) | ||
|
|
||
| // then | ||
| assertEquals(currentPage.captured, Pagination(page + 1)) |
There was a problem hiding this comment.
currentPage에 캡쳐되는 행위가 없는 것 같은데, 어떤 것을 검증한 것일까요?_?
또, 이 테스트에서는 slot을 활용하지 않고 테스트를 충분히 작성할 수 있어요. slot 없이 작성해보시길 바라요 :)
| @Test | ||
| internal fun 종료하면_화면을_닫는다() { |
There was a problem hiding this comment.
이 테스트를 제외하고 이 클래스의 모든 테스트 케이스가 fail이 나타나네요 😥
There was a problem hiding this comment.
테스트 코드가 실패하지 않도록 해보겠습니다!
|
말리빈 피드백주셔서 감사합니다 : ) 또한 말씀해주신 질문들도 충분히 고민해보는 시간을 갖도록 하겠습니다 😃 |
안녕하세요 말리빈 😃
지난 1, 2단계에서 주신 피드백을 다 반영하고 싶었지만 시간 관계상
더 보기 버튼에 대한 피드백은 아직 반영하지 못했습니다.. 😥또한 Dao에서 사용하지 않는 메서드를 다 제거하지 못하고 리뷰를 요청하여 해당 코드는 피드백을 반영하면서 함께 정리하겠습니다!
바쁘신 와중에 리뷰해주셔서 감사합니다 😃
이번 리뷰도 잘 부탁드립니다~!!