Student-Lyf / RamLife

An app for Ramaz students
https://ramaz-go.web.app
MIT License
5 stars 5 forks source link

Data refresh #110

Open todesj opened 2 years ago

todesj commented 2 years ago

First of all, you clearly understand the structure of a service and how to create one. Problem is, you don't really need to here. The root of the issue is that you're not accessing these timestamps for the sake of showing them in the app; you need them to manipulate other data (by refreshing them). Which means it shouldn't be its own service that your models call directly, but rather something that should be handled implicitly by the services layer, maybe in Database.init.

If you split your code into two parts, 1) deciding whether to update data, and 2) updating the data, you should only make part 1 implicit in Database.init. Remember that the user can always manually refresh the data in the UI, so you should make part 2 a separate function, perhaps a .update() on each HybridX.

All my comments in the code are either formatting or just an in-context explanation of the above.

OK I know that a service might be overkill here but it is really that bad if I use. a service instead so I don't have to change it?

Levi-Lesches commented 2 years ago

It's not really about overkill -- overkill is kinda the name of the game around organization. It's about separation. The reason we stopped getting these super-weird bugs is because the database is completely separate from the rest of the backend, which is completely separate from the fronted. Services are meant to help with that: each service gets a chance to init, and a Database is a special service that can also signIn. Each HybridDatabase is a special type of Database that can also manage two other Databases, a local and a cloud. But still, everything's separate. Just like services shouldn't interact with each other, you don't want databases interacting with each other either.

The way it's set up now, the refresh process is its own database that has control over every other database, which you don't really want. It's simpler to modify the definition of a DatabaseService to also include a function for updating, just like it already has functions for initialization and signing in. That way, each HybridDatabase can "keep its hands to itself" and only handle its own changes, while the master Database class can call the updates on each one. In that way it's like how each service has an init function but the master Services class goes to each service and calls them one-by-one.

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

Levi-Lesches commented 2 years ago

I cleaned up the commits by force-pushing. Some reviews are gone now but now it's good.