Conversation
nhyeonii
requested review from
Daljyeong,
Hyobeen-Park,
angryPodo and
nahy-512
and removed request for
a team
nahy-512
left a comment
nahy-512
left a comment
There was a problem hiding this comment.
고생하셨습니다!
PR에 내용 상세히 적어주신 덕에 작업 배경도 알게되고, 코드 읽기가 훨씬 수월해졌던 것 같아요
| implementation(libs.mlkit.text.recognition) | ||
| implementation(libs.kotlinx.coroutines.play.services) |
There was a problem hiding this comment.
오호 레포지토리를 새로 만드니까 얘네 의존성도 data > diary로 이동하는군요
There was a problem hiding this comment.
음...이 부분도 고민은 한번 해봤으면 해요. data:diary 모듈은 다이어리 데이터의 영속성을 관리하는 역할에 집중해야 한다고 생각해요. 지금은 '이미지에서 텍스트를 추출하는 기능' 이고 과연 Diary라는 도메인의 기능인가? 라는 의문이 생기네요. 장기적으로 보면 core:ml 혹은 data:recognition같은 모듈로 분리하는 과정이 필요할 수도 있을 것 같아요ㅎㅎ 그냥 고민만 해보셔
| import android.net.Uri | ||
|
|
||
| interface TextRecognitionRepository { | ||
| suspend fun extractTextFromImage(uri: Uri): Result<String> |
angryPodo
left a comment
angryPodo
left a comment
There was a problem hiding this comment.
아키텍쳐 관점에서 고민이랑 리소스 관리에 대해서 생각해볼만한 코멘트 남겼습니다. 기능에 크게 문제는 없어서 어푸해요👍🏻
| private val recognizer by lazy { | ||
| TextRecognition.getClient(TextRecognizerOptions.DEFAULT_OPTIONS) | ||
| } |
There was a problem hiding this comment.
크게 중요한건 아니지만 만약에 Impl을 단위 테스트 하고자 하면 ML kit 의존성을 모킹하기가 까다로울 수 있어요. Hilt모듈에서 @Provides로 제공하고 생성사 주입으로 변경하면 테스트 용이성이 좀 더 올라갑니다ㅎㅎ
생각해볼만한 주제여서 남겨요, 필수는 아님!👍🏻
There was a problem hiding this comment.
그리고 내부 코드를 까보면 TextRecongizer는 Closeable 인터페이스를 구현하고 있어요. 싱글톤 스코프로 주입받는 레포지토리 내부에서 lazy로 생성되면 앱 프로세스가 종료될 때까지 메모리를 차지할거에요. 일기를 여러번 작성한다면 살려놔도 좋을 것 같지만...여기도 고민되네요. 만약에 사용 빈도가 낮다고 판단되면 close()를 명시적으로 호출해서 자원을 해제하는 방법도 있습니다. 한번 고려해보셔요
| .onFailure { throwable -> | ||
| Timber.tag("DiaryWriteViewModel").e(throwable, "Text recognition failed") |
There was a problem hiding this comment.
혹시 태그가 꼭 필요하지 않다면 onLogFailure 사용해주세요~
| implementation(libs.mlkit.text.recognition) | ||
| implementation(libs.kotlinx.coroutines.play.services) |
There was a problem hiding this comment.
음...이 부분도 고민은 한번 해봤으면 해요. data:diary 모듈은 다이어리 데이터의 영속성을 관리하는 역할에 집중해야 한다고 생각해요. 지금은 '이미지에서 텍스트를 추출하는 기능' 이고 과연 Diary라는 도메인의 기능인가? 라는 의문이 생기네요. 장기적으로 보면 core:ml 혹은 data:recognition같은 모듈로 분리하는 과정이 필요할 수도 있을 것 같아요ㅎㅎ 그냥 고민만 해보셔
| it.copy( | ||
| diaryText = extractedText.take(MAX_DIARY_TEXT_LENGTH) | ||
| ) | ||
| .onFailure { throwable -> |
There was a problem hiding this comment.
오 저희 텍스트 추출에 실패했을 때 처리가 따로 안 되어 있었네요!? 이것도 다같이 고민해보면 좋을 것 같아요
There was a problem hiding this comment.
모듈을 따로 분리하지 않고 다이어리 모듈 안에 구현하신 이유가 궁금합니다!
저는 이 부분에 대해서 이미지 -> 텍스트 추출 기능이 data:diary 모듈에 딱 맞는 기능은 아닌 것 같아서 모듈 분리를 해야 하나 싶다가도 별도의 모듈로 분리했을 때 너무 과하게 모듈이 많아지는 것에 대해 우려가 되어 고민이 많이 되더라구요!
최근에 하이링구얼의 모듈 구조에 대해 계속해서 고민을 하던 중이라 나현이의 의견도 궁금하네요ㅎㅎ
There was a problem hiding this comment.
좋은 리뷰 감사합니다 ㅎㅎ 텍스트 인식이 diary 도메인의 핵심 책임은 아니라는 데 동의합니다. 다만 현재는 일기 작성에서만 사용되고 Repository로 이미 추상화되어 있어 실제 재사용 필요성이 생길 때 core:mlkit 같은 모듈로 분리해도
늦지 않을 것 같아서 일단 diary 모듈에 두었습니다. 향후 다른 기능에서도 필요해지면 말씀하신 방향으로 리팩토링 하는 것도 너무 좋을 것 같아요 😃
Daljyeong
left a comment
Daljyeong
left a comment
There was a problem hiding this comment.
확실히 ML Kit 로직을 분리해 주셔서 구조가 한층 깔끔해진 것 같습니다!
리뷰를 남길 게 거의 없어서🥲 개인적으로는 이미지->텍스트 추출 기능이 하이링구얼의 다른 기능으로 확장될 가능성은 크지 않을 것 같아 추후 수정이 필요해지는 시점에 모듈 분리를 검토해봐도 좋을 것 같다는 생각이 들었습니다. 추가로 답변 달아주시는 내용도 함께 팔로업 해보도록 하겠습니다 :)
이번 작업도 수고하셨습니다 🚀
5 participants
Related issue 🛠
Work Description ✏️
Uncompleted Tasks 😅
To Reviewers 📢
기존
DiaryWriteViewModel이 ML Kit의InputImage, TextRecognizer를 직접 생성하고 사용하는 구조였는데 이로 인해 두 가지 문제가 있었습니다.첫째로
ViewModel이@ApplicationContext를 직접 주입받아야 했고ViewModel이 Android 플랫폼에 강하게 결합되는 문제를 만들었습니다. 둘째로ML Kit텍스트 인식이라는 인프라 로직이ViewModel안에 섞이면서 단일 책임 원칙을 위반하고 있었습니다.이를 개선하기 위해
TextRecognitionRepository인터페이스를 정의하고ML Kit의존성은TextRecognitionRepositoryImpl안으로 캡슐화했습니다.ViewModel은 이제Uri를 넘기고Result<String>만 받으면 되기 때문에 ML Kit이 어떻게 동작하는지 알 필요가 없습니다.