Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extract methods to interfaces & change way of providing dependencies #25

Merged
merged 8 commits into from
Oct 30, 2023
Merged

Extract methods to interfaces & change way of providing dependencies #25

merged 8 commits into from
Oct 30, 2023

Conversation

hieuwu
Copy link
Collaborator

@hieuwu hieuwu commented Oct 28, 2023 •
edited
Loading

🚀 Summary

Extract methods to interfaces & change way of providing dependencies
Part 1 of #24

  • Avoid usage of manual singleton, replace with singleton component
  • Extract methods to interface to make the app ready for TDD

All new repository created must follow the principle programming over abstraction instead of implementation.
They should be provided in di and as a module

@hieuwu hieuwu requested review from kirillt and shubertm October 28, 2023 05:15
@hieuwu hieuwu self-assigned this Oct 28, 2023
@hieuwu hieuwu added the enhancement New feature or request label Oct 28, 2023
@hieuwu hieuwu temporarily deployed to Development October 28, 2023 05:21 — with GitHub Actions Inactive
shubertm
shubertm requested changes Oct 28, 2023
View reviewed changes
Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TextNotesRepo interface should be renamed NotesRepo and TextNotesRepoImpl to just TextNotesRepo. We will have other note type repos implement it

shubertm
shubertm reviewed Oct 28, 2023
View reviewed changes
Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should migrate to Datastore later
https://developer.android.com/topic/libraries/architecture/datastore

@hieuwu
Copy link
Collaborator Author

hieuwu commented Oct 29, 2023

TextNotesRepo interface should be renamed NotesRepo and TextNotesRepoImpl to just TextNotesRepo. We will have other note type repos implement it

Got your point. However, let's keep one implementation for one interface as it is current approach. If in the future we add more, let's consider later.

@hieuwu
Copy link
Collaborator Author

hieuwu commented Oct 29, 2023

We should migrate to Datastore later https://developer.android.com/topic/libraries/architecture/datastore

I just change the way to provide dependencies. If we need to migrate to data store, we should do it later

shubertm
shubertm approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@shubertm shubertm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepo.kt
}

private const val NOTE_EXT = "note"
interface TextNotesRepo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
interface TextNotesRepo {
interface NotesRepo {

app/src/main/java/space/taran/arkmemo/data/repositories/TextNotesRepoImpl.kt
import kotlin.io.path.name
import kotlin.io.path.writeLines

class TextNotesRepoImpl @Inject constructor(): TextNotesRepo {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class TextNotesRepoImpl @Inject constructor(): TextNotesRepo {
class TextNotesRepo @Inject constructor(): NotesRepo {

app/src/main/java/space/taran/arkmemo/di/RepositoryModule.kt
@Module
abstract class RepositoryModule {
@Binds
abstract fun bindRepository(impl: TextNotesRepoImpl): TextNotesRepo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
abstract fun bindRepository(impl: TextNotesRepoImpl): TextNotesRepo
abstract fun bindRepository(impl: TextNotesRepo): NotesRepo

app/src/main/java/space/taran/arkmemo/ui/viewmodels/NotesViewModel.kt
@@ -13,9 +13,11 @@ import space.taran.arkmemo.preferences.MemoPreferences
import javax.inject.Inject

@HiltViewModel
class NotesViewModel @Inject constructor(): ViewModel() {
class NotesViewModel @Inject constructor(
private val textNotesRepo: TextNotesRepo,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private val textNotesRepo: TextNotesRepo,
private val textNotesRepo: NotesRepo,

@hieuwu hieuwu merged commit 50558c5 into main Oct 30, 2023
1 check passed
@hieuwu hieuwu deleted the feature/improve-app-architecture branch October 30, 2023 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubertm shubertm shubertm approved these changes

@kirillt kirillt Awaiting requested review from kirillt

Assignees

@hieuwu hieuwu

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hieuwu @shubertm