[둘리] 2, 3단계 자동 DI 미션 제출합니다 #28

hyemdooly · 2023-09-08T15:19:44Z

안녕하세요, 베르! 리뷰 잘부탁드립니다. :)
프로젝트을 잠시 미뤄두고 미션에 집중한거라 천천히 리뷰해주셔도 괜찮습니다.
질문 있으시면 Comment 또는 슬랙 DM 보내주세요!

2단계 요구사항

필드 주입

@InMemory
class InMemoryProductRepository : ProductRepository {
    @Inject
    private val products: List<Product> = emptyList()

    override fun getAllProducts(): List<Product> {
        return products
    }
}

어디에 적용해볼까 고민하다가 InMemoryProductRepository에 products 프로퍼티에 적용해두었습니다.

// in DI
private fun <T> injectFields(instance: T) {
        val notNullInstance = requireNotNull(instance) { "Instance should not null" }
        val properties =
            notNullInstance::class.declaredMemberProperties.filter { it.hasAnnotation<Inject>() }

        properties.forEach { property ->
            property.isAccessible = true
            property.javaField?.let {
                val type = it.type.kotlin
                val fieldValue = Container.getInstance(type) ?: inject(type)
                it.set(instance, fieldValue)
            }
        }
    }

위 코드가 DI에서 필드 주입하는 코드입니다.
Inject annotation이 붙은 필드들만을 꺼내 주입해주고 있습니다.

Annotation

Annotation은 Qualifier, InMemory, InDisk로 정의해두었습니다.
Qualifier은 InMemory, InDisk에만 붙고, InMemory는 인메모리 레포지토리, InDisk는 데이터베이스 레포지토리에 붙습니다.
InDisk를 Database로 명칭을 짓지 않은 이유는, Room에도 InMemoryDatabase가 있는 것을 확인하고 명칭에 혼동이 있지 않을까 싶어서입니다.

Recursive DI

    fun <T> inject(modelClass: KClass<*>): T {
        val instance = Container.getInstance(modelClass) ?: createInstance(modelClass)
        return instance as T
    }

    private fun <T> createInstance(modelClass: KClass<*>): T {
        val constructor = modelClass.primaryConstructor
        requireNotNull(constructor) { "Unknown ViewModel Class $modelClass" }

        val paramInstances = getParamInstances(constructor)
        val instance = constructor.call(*paramInstances.toTypedArray()) as T
        injectFields(instance)

        return instance
    }

    private fun <T> getParamInstances(constructor: KFunction<T>): List<Any> {
        val paramInstances = constructor.parameters.map { param ->
            val type = param.type.jvmErasure
            Container.getInstance(type, param.annotations) ?: inject(type) // 재귀
        }
        return paramInstances
    }

    private fun <T> injectFields(instance: T) {
        val notNullInstance = requireNotNull(instance) { "Instance should not null" }
        val properties =
            notNullInstance::class.declaredMemberProperties.filter { it.hasAnnotation<Inject>() }

        properties.forEach { property ->
            property.isAccessible = true
            property.javaField?.let {
                val type = it.type.kotlin
                val fieldValue = Container.getInstance(type) ?: inject(type) // 재귀
                it.set(instance, fieldValue)
            }
        }
    }

주입하다가 Container에 원하는 인스턴스가 없으면 다시 inject를 호출해 재귀를 도는 방식입니다.

선택 요구 사항

삭제 버튼 클릭시 position에 해당하는 상품이 아니라 id에 해당하는 상품을 삭제하고 있습니다.
View에서 CartProductEntity가 아니라, 매핑하여 CartProduct를 참조하고 있습니다.

3단계 요구사항

Qualifier

class MainViewModel constructor(
    @InMemory private val productRepository: ProductRepository,
    @InDisk private val cartRepository: CartRepository,
) : ViewModel()

class CartViewModel constructor(
    @InDisk private val cartRepository: CartRepository,
) : ViewModel()

2단계에서 선언해둔 annotation을 사용하여, InMemory와 InDisk를 설정하여 주입받을 수 있습니다.
DI에서는 해당 annotation이 붙은 Repository 구현체를 찾아 주입합니다.

추가로 현재 CartViewModel은 InDisk Repository를 받고 있으나, 테스트에는 InMemory의 경우도 작성되어있으므로 참고해주세요!

모듈 분리

모듈 분리 했습니다!

선택 요구 사항

DI 라이브러리 배포 완료 및 적용했습니다. (DSL 제외 적용 완료)

그 외

아시다시피 ViewModel 클래스에서 생성자 옆에 constructor를 붙이지 않으면 DI에서 Property의 InMemory, InDisk annotation이 받아지지 않습니다.
아직 원인을 파악하지 못해서 두었습니다..! 디컴파일해서 봐야할 것 같아요.🥲
참고해주시고 혹시 리뷰하시다가 이유를 발견하신다면 공유 부탁드립니다!!

…할 수 없다는 문제를 해결한다

woowahan-pjs

전체적인 설계와 깔끔한 코드 구현이 좋았습니다. 👍
고민해 보면 좋을 것 같은 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!
당장은 @InMemory, @InDisk로 나누어서 사용하면 도움이 될 수 있겠지만, 조금 더 생각해 보면 컴포넌트에 이름을 지정하고 @Qualifier가 해당 이름으로 컴포넌트를 주입하도록 하는 것은 어떨까요? 주입 대상이 반드시 데이터베이스에만 국한되는 것은 아니라고 생각합니다.
그 외에도 몇 가지 의견을 남겨 놓았으니 충분히 고민하고 도전해 보세요!

woowahan-pjs · 2023-09-09T03:22:34Z

app/build.gradle.kts

@@ -44,13 +44,16 @@ android {

 dependencies {
 implementation(project(":domain"))
+ implementation("com.github.hyemdooly:android-di:v1.0.0")
+// implementation(project(":di"))


주석을 해제해야 하지 않을까요?

둘리가 배포한 라이브러리 버전에는 원하는 기능이 포함되어 있지 않네요.

배포한걸 implementation 했으니 주석처리해도 된다고 생각했습니다!
배포한 라이브러리에 원하는 기능이 포함되어있지 않다니 확인해보겠습니다.ㅠㅠ

세상에 도메인 모듈에 퍼블리싱이 되어있었네요.................. 감사합니다 제이슨!!

woowahan-pjs · 2023-09-09T03:22:43Z

app/src/main/java/woowacourse/shopping/ui/ShoppingApplication.kt

+ Container.addInstance(List::class, products)
+ Container.addInstance(CartProductDao::class, inDiskDb.cartProductDao())
+
+ Container.addInstance(ProductRepository::class, Injector.inject(InMemoryProductRepository::class))
+ Container.addInstance(CartRepository::class, Injector.inject(InDiskCartRepository::class))
+ Container.addInstance(CartRepository::class, Injector.inject(InMemoryCartRepository::class))


컴포넌트로 등록하려는 대상 클래스에 추가한 애너테이션을 활용하면 이와 같은 코드를 없애고 라이브러리를 더 사용자 친화적으로 만들 수 있을 거예요. 아래의 글이 도움 될 거예요.
https://www.baeldung.com/reflections-library

woowahan-pjs · 2023-09-09T03:24:18Z

app/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.kt


-class CartViewModel(
- private val cartRepository: CartRepository,
+class CartViewModel constructor(


저는 constructor를 제거했는데 정상적으로 작동하네요.

저도 리뷰보고 제거해봤는데 되네요...? 제 사라진 2시간을 찾습니다...🥲

woowahan-pjs · 2023-09-09T03:24:20Z

app/src/main/java/woowacourse/shopping/ui/cart/CartViewModel.kt

-class CartViewModel(
- private val cartRepository: CartRepository,
+class CartViewModel constructor(
+ @InDisk private val cartRepository: CartRepository,


아래는 사용자 코드의 예시이므로 참고만 하세요!

Suggested change

@InDisk private val cartRepository: CartRepository,

@Qualifier("inDiskCartRepository") private val cartRepository: CartRepository,

SeongHoonC

둘리! 고생 많으셨습니다ㅎㅎ 리뷰가 늦었네요ㅜ Recursive DI 가 구현되지 않았습니다. 코멘트가 좀 많은데 step4 할 시간이 부족할 것 같다 판단되면 말씀해주세요!

SeongHoonC · 2023-09-14T01:48:40Z

app/src/main/java/woowacourse/shopping/data/mapper/CartProductMapper.kt

+import woowacourse.shopping.model.CartProduct
+
+fun CartProductEntity.toCartProduct(): CartProduct {
+ return CartProduct(id, name, price, imageUrl, createdAt)


함수에 인자가 많을 때는 파라미터 명을 나타내는 것이 바람직하다고 생각합니다.
현재는 이름이 똑같아서 알아보는데 무리는 없네요.
변경 가능성을 고려하는 것도 좋을 것 같아요!

추가했습니다~

SeongHoonC · 2023-09-14T01:51:07Z

domain/src/main/java/woowacourse/shopping/model/CartProduct.kt

@@ -0,0 +1,3 @@
+package woowacourse.shopping.model
+
+data class CartProduct(val id: Long, val name: String, val price: Int, val imageUrl: String, val createAt: Long)


DB 의 Entity 가 아닌 도메인 모델에 createAt 이라는 네이밍이 적절한가요?

추가된 시간이라는 의미의 timeAdded로 변경했습니다!

SeongHoonC · 2023-09-14T01:55:59Z

app/src/main/java/woowacourse/shopping/ui/cart/CartProductViewHolder.kt

@@ -19,9 +19,10 @@ class CartProductViewHolder(
 }
 }

- fun bind(product: Product) {
+ fun bind(product: CartProduct) {


변수명이 product 와 혼동할 수 있을 것 같아요!

cartProduct로 수정했습니다!

SeongHoonC · 2023-09-14T02:08:08Z

di/src/main/java/io/hyemdooly/di/Injector.kt

+ if (annotation != null) {
+ Container.getInstance(annotation.clazz) ?: inject(annotation.clazz)
+ } else {
+ Container.getInstance(type) ?: inject(type)
+ }


같은 함수에 대한 분기를 Container 외부에서 하고있네요.
Container 가 자신의 책임을 다할 수 있도록 능동적으로 협력할 수 있게 작성하는 것이 어떨까요?

Annotation을 뜯어 타입을 확인하는 것을 Container 내부에서 하게 하려고 했으나,
내부에서 annotation.clazz or param.type.jvmErasure로 인스턴스를 찾을 때 없는 경우 다시 inject를 해주기 위해 null을 반환하게 되는데요!
이런 경우 annotation.clazz과 param.type.jvmErasure 중 어떤 타입으로 inject를 해주어야할지 알 방법이 없었습니다.
그리고 Annotation을 뜯는 것은 DI의 몫이라고 생각해서 대신에 중복 코드를 제거했습니다!

SeongHoonC · 2023-09-14T02:10:53Z

di/src/main/java/io/hyemdooly/di/Injector.kt

+ }
+
+ private fun <T> injectFields(instance: T) {
+ val notNullInstance = requireNotNull(instance) { "Instance should not null" }


notNullInstance 라는 변수명은 컴파일러가 NullSafety 를 검증하는 코틀린을 사용함에 있어 적합하지 않은 변수명이라고 생각합니다.
nullable 하지 않다고 명시하는 다른 방법은 없을까요?

Non null임을 명시하기 위해 T : Any로 수정했습니다.

SeongHoonC · 2023-09-14T02:33:28Z

app/src/main/java/woowacourse/shopping/ui/ShoppingApplication.kt

+ val inDiskDb = Room.databaseBuilder(
+ applicationContext,
+ ShoppingDatabase::class.java,
+ ShoppingDatabase.name,
+ ).build()
+
+ val products = listOf(
+ Product(
+ name = "우테코 과자",
+ price = 10_000,
+ imageUrl = "https://cdn-mart.baemin.com/sellergoods/api/main/df6d76fb-925b-40f8-9d1c-f0920c3c697a.jpg?h=700&w=700",
+ ),
+ Product(
+ name = "우테코 쥬스",
+ price = 8_000,
+ imageUrl = "https://cdn-mart.baemin.com/sellergoods/main/52dca718-31c5-4f80-bafa-7e300d8c876a.jpg?h=700&w=700",
+ ),
+ Product(
+ name = "우테코 아이스크림",
+ price = 20_000,
+ imageUrl = "https://cdn-mart.baemin.com/sellergoods/main/e703c53e-5d01-4b20-bd33-85b5e778e73f.jpg?h=700&w=700",
+ ),
+ )
+
+ Container.addInstance(products)
+ Container.addInstance(inDiskDb.cartProductDao())
+
+ Container.addInstance(Injector.inject(InMemoryProductRepository::class))
+ Container.addInstance(Injector.inject(InDiskCartRepository::class))
+ Container.addInstance(Injector.inject(InMemoryCartRepository::class))


현재 구조에서는 주입해야하는 것들이 생길 때마다 Application 을 수정해야하네요.
주입해야하는 것이 100개라면 application class 가 너무 커지지 않을까요?

Module을 생성하여 묶어서 instances에 추가할 수 있도록 수정했습니다.

SeongHoonC · 2023-09-14T02:36:00Z

app/src/main/java/woowacourse/shopping/ui/ViewModelFactory.kt

+val viewModelFactory = object : ViewModelProvider.Factory {
+ override fun <T : ViewModel> create(modelClass: Class<T>, extras: CreationExtras): T =
+ Injector.inject(modelClass.kotlin)
+}


라이브러리 사용자가 ViewModelFactory 의 주입을 정의하고 사용해야하네요. 자동으로 할 수 있지 않을까요?

고민을 해봤는데 DI는 안드로이드에서만 사용되는 개념이 아니라고 생각합니다.
따라서, 어떻게 사용하든 사용자가 직접 정의해야한다고 생각해요!
DI가 안드로이드 의존성을 가져야할까요? 다음 단계에서도 안드로이드 의존성 없이 구현해보려고 합니다!

SeongHoonC · 2023-09-14T02:43:20Z

di/src/test/kotlin/io/hyemdooly/di/InjectorTest.kt

+ assertNotNull(viewModel)
+ assertNotNull(viewModel.fakeRepository)
+ assertEquals(name, viewModel.fakeRepository.name)
+ assertEquals(items, viewModel.fakeRepository.items)


테스트에 실패하면 어떤 테스트가 실패했는지 알 수 없어요. SoftAssertion 이나 assertAll 을 사용해보는게 어떨까요?

assertAll 적용했습니다!

SeongHoonC · 2023-09-14T03:00:36Z

di/src/main/java/io/hyemdooly/di/Injector.kt

+ val type = it.type.kotlin
+ val fieldValue = Container.getInstance(type) ?: inject(type)
+ it.set(instance, fieldValue)


필드 주입할 때 type 으로 찾는데 기본 자료형이면 겹치는 자료형은 최신에 넣은 값을 가져와 의도치 않은 결과가 발생할 것 같네요.

@Test fun `Container에서 타입에 맞는 instance를 찾아 의존성을 주입한다`() { // given val name = "FakeRepository" val fail = "fail" // when Container.addInstance(name) Container.addInstance(fail) val viewModel = Injector.inject<FakeViewModel>(FakeViewModel::class) // then // ... assertEquals(name, viewModel.fakeRepository.name) // fail }

@Inject(val name: String)으로 해보려다가 개발자가 직접 String으로 구분하는 것은 오타의 위험이 큰 것 같아 고려하지 않기로 했습니다...
primitive type을 걸러보려고 했는데, primitive type외에도 List와 같은 형태도 같은 결과를 가져올 것 같아 좀 더 고민이 필요할 것 같습니다.
그에 따라 테스트 코드도 수정했습니다.

SeongHoonC · 2023-09-14T03:07:08Z

app/src/main/java/woowacourse/shopping/ui/ShoppingApplication.kt

+ Container.addInstance(products)
+ Container.addInstance(inDiskDb.cartProductDao())
+
+ Container.addInstance(Injector.inject(InMemoryProductRepository::class))
+ Container.addInstance(Injector.inject(InDiskCartRepository::class))
+ Container.addInstance(Injector.inject(InMemoryCartRepository::class))


주입 순서가 바뀌면 주입에 실패하네요! recursive DI 가 안되고 있습니다ㅜ

삭제해야하는 코드였는데 삭제하지 않았네요... 삭제했습니다 ㅎㅎ

SeongHoonC

리뷰 반영된 것 확인했습니다! 리뷰가 많았는데 짧은 시간안에 반영하다니 대단합니다ㅎㅎ
추가로 코멘트 남겼으니 확인해보시고 step4 에 반영할지 고민해보셨으면 좋겠습니다! step4 화이팅입니다!

SeongHoonC · 2023-09-14T14:17:42Z

app/src/main/java/woowacourse/shopping/ui/di/DaoModule.kt

+import woowacourse.shopping.data.CartProductDao
+import woowacourse.shopping.data.ShoppingDatabase
+
+class DaoModule(private val context: Context) : Module {


Dao 뿐만 아니라 context 를 필요로 하는 객체는 여기서 생성할 수 있지 않을까요? DaoModule 보다는 더 좋은 이름이 있을 것 같아요!

SeongHoonC · 2023-09-14T14:19:19Z

app/src/main/java/woowacourse/shopping/ui/di/DaoModule.kt

+
+class DaoModule(private val context: Context) : Module {
+ fun provideCartProductDao(): CartProductDao = Room.databaseBuilder(
+ context,


Room 이 applicationContext 를 사용하도록 강제할 순 없을까용?

추가로 영속성 데이터 저장소는 싱글톤으로 관리하는게 좋아요!

SeongHoonC · 2023-09-14T14:37:20Z

di/src/test/kotlin/io/hyemdooly/di/InjectorTest.kt

object 주입이나 인자가 여러개인 경우 등 다른 경우의 수를 추가해서 테스트 해봤는데 잘 작동하는 것 확인했습니다!

hyemdooly added 30 commits September 7, 2023 13:49

docs: 요구사항 업데이트

168589f

docs: 요구사항 업데이트

87d3c2a

feat: 요구사항에 맞게 CartRepository 변경

657518e

feat: CartProduct 생성

1bdceaf

rename: Repository 네이밍 변경

5e94a5e

feat: Room 사용 구현

7ab7e4e

refactor: Inject 로직 분리

04ac319

refactor: Inject 로직 분리

c75db5c

refactor: viewModelFactory 수정

f483bbb

feat: Field 주입에 사용할 Annotation 추가

d2744a7

refactor: getInstance Nullable 허용

c6393e6

refactor: Field 주입으로 변경

8b64b40

feat: Recursive DI 구현

ffa5c7b

feat: Recursive DI, Field Injection 적용

2254b53

docs: 요구사항 적용 여부 업데이트

8d57300

test: Injector Test 작성

e2b9366

feat: 상품의 position과 CartRepository::deleteCartProduct의 id가 동일한 값임을 보장…

62465b1

…할 수 없다는 문제를 해결한다

feat: 구분을 위한 추가 Annotation 생성

559b93f

feat: ViewModel 프로퍼티에 annotations 지정

f6c41a8

refactor: 필요없는 괄호 삭제

c0ee682

feat: injector annotation 고려

38725ee

feat: InMemory, Database 레포지토리 구현

55308d0

feat: 바뀐 요구사항에 맞게 주입 수정

a80f744

feat: annotation 고려를 위한 instances 수정

15b4fe6

test: 바뀐 코드에 맞게 test 수정

37d31ed

docs: 요구사항 충족 여부 수정

7f5c22d

rename: DI 모듈화

368e9c9

docs: 모듈화 충족 여부 수정

83f960e

test: ViewModel 주입 테스트 수정

6f0dcf2

test: di module에 InjectorTest 구현

d66108d

hyemdooly requested a review from SeongHoonC September 8, 2023 15:19

woowahan-pjs requested changes Sep 9, 2023

View reviewed changes

hyemdooly added 12 commits September 10, 2023 15:47

refactor: constructor 제거

2498638

deploy: DI 배포 수정

9eecad2

deploy: DI 배포 수정

8a46ecd

refactor: di 버전 수정

8fca93c

refactor: Qualifier 수정, DI 수정

a636e8c

refactor: println() 삭제

7184501

deploy: publishing 옵션 변경

7443924

refactor: implementation 추가

353d48b

deploy: di release

bbb76a5

deploy: di release

a89cc44

test: 바뀐 di에 따른 테스트 변경

acaa5ef

refactor: 배포된 di 적용

e6ee91f

otter66 mentioned this pull request Sep 14, 2023

[베리] 2, 3단계 자동 DI 미션 제출합니다 #46

Merged

15 tasks

SeongHoonC requested changes Sep 14, 2023

View reviewed changes

hyemdooly added 12 commits September 14, 2023 13:58

refactor: 전단계의 잔해 삭제

1f27660

refactor: instance return 수정

d53b175

refactor: 파라미터명 명시

885ab4d

refactor: 변수명 변경

fa81116

refactor: 파라미터명 변경

77bc755

refactor: 중복 코드 제거

ad360bb

refactor: T 타입 non null로 변경

55bc13c

refactor: scope function 적용

cc6cc2e

refactor: id 계산법 수정

6c59beb

refactor: assertAll 적용

a7baa40

refactor: kotlin class type 고려 제외

47600c6

refactor: Module 개념 추가

a91d38b

SeongHoonC approved these changes Sep 14, 2023

View reviewed changes

SeongHoonC merged commit 5da6d9b into woowacourse:hyemdooly Sep 14, 2023

	@InDisk private val cartRepository: CartRepository,
	@Qualifier("inDiskCartRepository") private val cartRepository: CartRepository,

		@@ -0,0 +1,3 @@
		package woowacourse.shopping.model

		data class CartProduct(val id: Long, val name: String, val price: Int, val imageUrl: String, val createAt: Long)

[둘리] 2, 3단계 자동 DI 미션 제출합니다 #28

[둘리] 2, 3단계 자동 DI 미션 제출합니다 #28

Conversation

hyemdooly commented Sep 8, 2023 • edited

2단계 요구사항

필드 주입

Annotation

Recursive DI

선택 요구 사항

3단계 요구사항

Qualifier

모듈 분리

선택 요구 사항

그 외

woowahan-pjs left a comment • edited

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

woowahan-pjs Sep 9, 2023 • edited

Choose a reason for hiding this comment

SeongHoonC 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

SeongHoonC 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

hyemdooly commented Sep 8, 2023 •

edited

woowahan-pjs left a comment •

edited

woowahan-pjs Sep 9, 2023 •

edited