GEWIS / sudosos-backend

SudoSOS is a Node.js-based Bar and POS system made for study association GEWIS.
https://sudosos.gewis.nl
GNU Affero General Public License v3.0
4 stars 3 forks source link

Refactoring service classes to improve cross service usability. #153

Open JustSamuel opened 7 months ago

JustSamuel commented 7 months ago

In this pull request I also refactored the service class, since it was long over due.

However, given that I wrote quite a lot of the current service classes they mostly have the same issues. Service files / classes should only consider database entities, it is up to the controller to turn this entity into a response.

This prevents the issue I had where you go from service -> response -> back to an entity. Wasting database queries and muddling up the code.

A following issue that I found is that there is a sneaky form of code duplication. Namely that we use User.find() in quite a lot of places, and do not use the UserService. Which is fine, however, lets say we want to implement account expiry we would have to refactor a lot of places just to add something along the lines of expired: false to the find options.

Another code smell this solves is the .records being used because most service functions return a paginated response, meaning we are wasting computing power on creating pagination just to throw it away.

To solve this I propose the following:

Yoronex commented 2 months ago

Another code smell this solves is the .records being used because most service functions return a paginated response, meaning we are wasting computing power on creating pagination just to throw it away.

This is not true, because when you do not give any pagination parameters, pagination is not actually applied.

JustSamuel commented 1 month ago

This is not true, because when you do not give any pagination parameters, pagination is not actually applied.

I am still of the opinion that .records is smelly, .records[0] is also super ugly.

Yoronex commented 1 month ago

Then maybe as a middleground, we can implement a response like TypeORM does with .findAndCount(). I think that looks quite neat.

JustSamuel commented 22 hours ago

Note that this refactor per service should also include moving to a class "manager" and away from the global TypeORM manager.