TechDebt: Remove overloaded status enums. #143
Comments
|
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
<#143>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADBPWPA33DVS6R5WEEMYG6TTKQF5FANCNFSM43RIDGEQ>
.
|
|
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. |
|
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
<#143 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADBPWPFYHHEDCSOQSQFYMJTTKQJEZANCNFSM43RIDGEQ>
.
|
|
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 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. |
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.The text was updated successfully, but these errors were encountered: