search

SimplQ / simplQ-backend

SimplQ backend, written in Java for AWS
https://simplq.me
GNU General Public License v3.0
17 stars 27 forks source link

TechDebt: Remove overloaded status enums. #143

Open daltonfury42 opened 3 years ago

daltonfury42 commented 3 years ago

Our backend is very straightforward CRUD on two resources, queues and tokens. Both of them have two status fields, QueueStatus and TokenStatus.

These enums are a bit overloaded, a better approach would be remove them and have booleans, (isPaused, isRemoved) for queues, and (isRemoved, isNotified) for tokens.

thehamzarocks commented 3 years ago

Hmm I feel these Booleans are too tightly coupled? A queue can only have one of isPaused or isRemoved anyway? And if we add another status, we might need to add more booleans if we go by this approach?

On Sun, 25 Apr 2021 at 6:21 PM, daltonfury42 @.***> wrote:

Our backend is very straightforward CRUD on two resources, queues and tokens. Both of them have two status fields, QueueStatus https://github.com/SimplQ/simplQ-backend/blob/master/simplq/src/main/java/me/simplq/constants/QueueStatus.java and TokenStatus https://github.com/SimplQ/simplQ-backend/blob/master/simplq/src/main/java/me/simplq/constants/TokenStatus.java .

These enums are a bit overloaded, a better approach would be remove them and have booleans, (isPaused, isRemoved) for queues, and (isRemoved, isNotified) for tokens.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/SimplQ/simplQ-backend/issues/143, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBPWPA33DVS6R5WEEMYG6TTKQF5FANCNFSM43RIDGEQ .

daltonfury42 commented 3 years ago

My reasoning was that "removing/deleting" and "pausing" are two different unrelated things, and should not be kept coupled. It could lead to bugs like you pause->delete->resume, and it would inadvertently lead to it getting undeleted, which might not be the intended outcome. Then we would have to handle it using if/else in code, as we are currently doing. (We could avoid a lot of bad if/else here).

Then we could have isPaused set via our PATCH request (here), and delete can be modeled as a DELETE request.

thehamzarocks commented 3 years ago

Hmm true if there was only one status flag we’d need to check if the queue was deleted before resuming. But with two flags, wherever we need to check if the queue is not paused, we’d need to check that it was also not deleted? 🤔 Although that would be a simple Boolean condition anyway. My concern with more states still applies but if we don’t see that happening, this should be fine.

On Sun, 25 Apr 2021 at 6:49 PM, daltonfury42 @.***> wrote:

My reasoning was that "removing/deleting" and "pausing" are two different unrelated things, and should not be kept coupled. It could lead to bugs like you pause->delete->resume, and it would inadvertently lead to it getting undeleted, which might not be the intended outcome. Then we would have to handle it using if/else in code, as we are currently doing. (We could avoid a lot of bad if/else here https://github.com/SimplQ/simplQ-backend/blob/master/simplq/src/main/java/me/simplq/service/QueueService.java#L62 ).

Then we could have isPaused set via our PATCH request (here https://github.com/SimplQ/simplQ-backend/blob/master/simplq/src/main/java/me/simplq/controller/QueueController.java#L47), and delete can be modeled as a DELETE request.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/SimplQ/simplQ-backend/issues/143#issuecomment-826323416, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADBPWPFYHHEDCSOQSQFYMJTTKQJEZANCNFSM43RIDGEQ .

daltonfury42 commented 3 years ago

In this case, I was reading our code, and while counting the number of people currently in the queue, we were taking count of tokens where status = WAITING. We missed the people who were NOTIFIED. They are also in the queue and should have been counted.

So, same reasoning, if the token was notified or not, is independent of if the token is currently in the queue or not (DELETED vs ACTIVE).

You could say that we could have taken the count(status != DELETED), but still...

Also we might start sending SMS as notifications soon (if all goes well, 50% chance), and tomorrow if we want to count the number of SMSes that were send by the user etc, it's better to keep them as separate fields.