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.

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

Add data models, refactor local and network data sources #908

Merged
merged 8 commits into from Mar 7, 2023

Conversation

dturner
Copy link
Collaborator

@dturner dturner commented Mar 3, 2023

Here's what I've done and why:

Add internal data models
These are:

  • NetworkTask represents a task obtained from the network
  • LocalTask represents a task stored in a local Room database

I've also added mapping functions to map between the two new models and the original Task model, which is exposed by the data layer to the UI layer. This is to more accurately mirror what would happen in a real app, where data obtained from a particular source needs to be mapped into a format which can be used by the rest of the app.

They are in ModelMappingExt.kt, and could be converted to mapping classes if this is better practice.

Local data source is now a Data Access Object
Having both the local and network data sources implement the same interface wasn't realistic (they wouldn't have the same CRUD methods). The local data source is now a DAO.

Task creation and updates are now handled by the task repository
I've moved the responsibility for creating and updating tasks into the task repo (with createTask and updateTask methods). Previously the UI layer (or any other layer) could create or update a Task, then the repo would save it. This violates SSOT because it's possible to have tasks with UUIDs in memory which the repository hasn't ever seen. This way the UUID creation is encapsulated in the repository, although it does mean the create and update methods need updating each time a new editable field is added to Task. Feedback welcome on this.

Tidied a few other things up

  • Removed FakeFailingTasksRemoteDataSource.kt - it's not used anywhere and I got tired of having build failures every time I changed the data source interface which it implements
  • Removed the repo.*blocking convenience methods - tests have been updated to use runTest instead

Copy link
Contributor

@tunjid tunjid left a comment

Choose a reason for hiding this comment

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

LGTM.

@dturner
Copy link
Collaborator Author

dturner commented Mar 6, 2023

Thanks @tunjid. Did you see my comments about the independent coroutines? This is the existing functionality - although it looks like new code it's actually exactly the same as how it works currently. I will address this in a separate PR if/when we redo the network data source and sync algorithm.

Copy link
Contributor

@manuelvicnt manuelvicnt left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@mjobayeraraf
Copy link

mjobayeraraf commented Mar 7, 2023 via email

@dturner dturner merged commit f01fcad into android:main Mar 7, 2023
@dturner dturner deleted the add-data-models branch March 7, 2023 13:10
*/

// External to local
fun Task.toLocalModel() = LocalTask(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If this app were modularized, we would have the mappers in different modules. I'd expect these mappers to live in the boundaries between the components that use them. Some in data/source/local and some in data/source/network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants