Handle updating category name regardless of whether it's preset or stored #460
Conversation
PaulKlauser
requested review from
nicholas-cook,
jrtb,
stechkev,
nerdsosky,
jollygreenegiant,
willdenne and
caroline-wang-320
as code owners
| } else { | ||
| val presetCategory = presetCategoriesRepository.getCategoryById(categoryId) | ||
| if (presetCategory != null) { | ||
| presetCategoriesRepository.hidePresetCategory(categoryId) | ||
| storedCategoriesRepository.upsertCategory( | ||
| Category.StoredCategory( | ||
| presetCategory.categoryId, | ||
| localizedName, | ||
| presetCategory.hidden, | ||
| presetCategory.sortOrder | ||
| ) | ||
| ) | ||
| } else { | ||
| throw IllegalArgumentException("Category with id $categoryId not found") | ||
| } | ||
| } |
There was a problem hiding this comment.
Just curious- is this a solved problem? Throw exception vs. return result (vs silently swallow/log errors 👀)
If you've got any sage words or content you could point me to about this I'd appreciate it. Otherwise I can just research it a little and discuss with the smart folks at office hours tomorrow
There was a problem hiding this comment.
My stance on exceptions is generally aligned with this guy.
In this case, this method being called with a category ID that cannot be found is not an expected state of our application. It could be argued that we don't want the app to crash in this case, but I'd also argue that there's a benefit to a crash in that it's very obvious something has gone wrong, and requires the least lift right now.
I think at the least we'd want a result return type, so that we could communicate to the caller that things broke, so that it can react accordingly. If we fail silently, we may leave the user stuck in an invalid state, which is worse than a crash IMO.
Granted, as I type this, perhaps a crash in Vocable is worst case, because the user may not be able to launch Vocable again themselves if they have limited mobility.
What are your thoughts?
PaulKlauser
deleted the
chore/452-abstract-over-saved-preset-categories-5
branch
#452