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

Architecture/data layer #8

Merged
merged 10 commits into from
Mar 29, 2021
Merged

Conversation

matuella
Copy link
Contributor

Adds the data layer core pieces, such as DatabaseRepository and its implementation with sembast.

Unit tests on most of cases are also done.

@matuella matuella requested a review from ggirotto March 26, 2021 20:50
@matuella matuella self-assigned this Mar 26, 2021
Copy link
Contributor

@ggirotto ggirotto left a comment

Choose a reason for hiding this comment

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

Although the DatabaseRepositoryImpl receives a database instance on its constructor, this data layer is not ready to handle multiple database instances. Have you though about this multiple databases necessity? How would you manage this in the current structure?

}

/// Transforms a list of `sembast` snapshot records into a list of objects parsed by [serializer]
StreamTransformer<List<RecordSnapshot<String, Map<String, Object?>>>, List<T>>
Copy link
Contributor

@ggirotto ggirotto Mar 29, 2021

Choose a reason for hiding this comment

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

Maybe creating a single or multiple custom types to this return? This function signature is unreadable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way to create custom types that aren't functions. This might be possible in 2.13, but not right now

@matuella
Copy link
Contributor Author

@ggirotto

Although the DatabaseRepositoryImpl receives a database instance on its constructor, this data layer is not ready to handle multiple database instances. Have you though about this multiple databases necessity? How would you manage this in the current structure?

The idea is that we should not need to instantiate multiple databases just for the sake of performance. If there is a problem with performance, even if they are mitigated when split into multiple files, those problems will inevitably occur even if a single DB has too many elements. So if this is a problem, there are plausible workarounds, like parameterizing the constructor/openDB function. Although the underlying problem (which is the library not being performant with a heavy load of elements) is what should be solved instead, not creating multiple DB instances.

With this in mind, I have my doubts that it will be really necessary in the future, as we should eventually need to go to cloud to provide such amount of data, and not storing thousands of MBs for decks that the user will never use (possibly the majority) on the device.

@matuella matuella requested a review from ggirotto March 29, 2021 13:59
@matuella matuella merged commit 9fcc000 into memo-architecture Mar 29, 2021
@matuella matuella mentioned this pull request Mar 30, 2021
@matuella matuella deleted the architecture/data-layer branch April 11, 2021 14:19
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

3 participants