-
Notifications
You must be signed in to change notification settings - Fork 1
[김예란_Android] 8주차 과제 제출 #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
kongwoojin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
전에 비해서 확실히 실력이 느신게 보이네요
몇가지 피드백을 드리자면
- 현재 구조는 클린 아키텍처를 따른다고 보기는 어렵습니다. 클린 아키텍처는 domain layer가 필수지만, 현재 구조상 data 와 presentation 레이어만 존재하고, Repository와 Usecase, DataSource도 없네요.
- 의존성 주입을 사용하는 것이 좋습니다.
AndroidViewModel을 사용하신 이유가Room에 context를 주입하기 위해서일텐데,AndroidViewModel은 사용을 지양하는 것이 맞고, 결국 의존성 주입을 사용하면 해결이 가능합니다. - 이건 팁인데, 사람들이 클린 아키텍처랑 앱 아키텍처에 대해 많이 헷갈려합니다. 이것도 한번 어떤 차이점이 있는지 알아보시면 좋을 것 같아요.
| viewBinding = true | ||
| dataBinding = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 질문! viewBinding과 dataBinding은 무슨 차이일까요?
| suspend fun delete(word: WordEntity) | ||
|
|
||
| @Query("SELECT * FROM word_table") | ||
| fun getAll(): LiveData<List<WordEntity>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LiveData는 안드로이드 생명주기를 인식하고 있는 클래스입니다.
따라서, 안드로이드 생명주기 밖에서 동작하는 data layer에서 사용하기는 부적절해보입니다.
| intent.putExtra(EXTRA_WORD_ID, word.id) | ||
| intent.putExtra(EXTRA_WORD, word.word) | ||
| intent.putExtra(EXTRA_MEANING, word.meaning) | ||
| intent.putExtra(EXTRA_IMAGE_PATH, word.imagePath) | ||
| startActivity(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bundle을 사용하지 않은 이유가 있을까요?
| private lateinit var wordViewModel: WordViewModel | ||
| private var wordId: Int? = null | ||
| private var selectedImageUri: Uri? = null | ||
| lateinit var pickImage: ActivityResultLauncher<String> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private 처리해주세요
|
|
||
| if (wordId != null && wordId != -1) { | ||
| val wordEntity = WordEntity( | ||
| id = wordId!!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wordId != null이 선언되었으니 타입 추론이 되지 않나요?
| import data.WordEntity | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| class WordViewModel(application: Application) : AndroidViewModel(application) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 AndroidViewModel를 사용하신 이유가 있을까요?
| androidx-room-compiler = { module = "androidx.room:room-compiler", version.ref = "roomCommonJvm" } | ||
| androidx-room-ktx = { module = "androidx.room:room-ktx", version.ref = "roomCommonJvm" } | ||
| androidx-room-runtime = { module = "androidx.room:room-runtime", version.ref = "roomCommonJvm" } | ||
| coil = { module = "io.coil-kt:coil", version.ref = "coil" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 Glide 대신 Coil을 사용하신 이유가 무엇인가요?
No description provided.