Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates update logic to require animation id in url. #8

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

chris-schmitz
Copy link
Owner

@chris-schmitz chris-schmitz commented Jan 2, 2024

While working on the remaining crud of the API I debated on if the animation's ID should be in the animation data class or out on it's own when updating an existing record. At the time I decided to keep it with the DTO and just use the HTTP verb to denote the update vs insert.

Then I started to dive back into the client side code and realized that I wanted to be more explicit with the ID as part of the request. Either approach is still valid and debatable, but there were two main factors that made me decide to switch direction:

A natural validation check

The id being required in the URL means that we have a natural validation check for the request's required information.

i.e. if you don't provide the critical bit of info to do the update (the record id to update) then you can't even successfully make the API call. It also means that we don't have to explicitly interrogate the Animation data class to check to see if the id is missing or not. It's a nullable field in the data class at the moment, so missing the data wouldn't cause an error so you have to explicitly look for it and throw an error. Again, with the ID in the url the check is just part of making the call.

Clear code readability for the engineer

The other part that's a bit less critical and a bit more arguable is the readability from the front end.

Screenshot 2024-01-01 at 9 05 18 PM

Here we have the helper tooling to make the save vs the update API calls. They're practically identical (which makes sense considering what each is doing), and the big distinguishing characteristics are the http method used, but also the ID standing out visually.

Really, we could compact the two methods and just programmatically switch based on if the front end has the animation ID or not (and eventually we'll likely do that), but even still I think the ID in the url on the font end forces the engineer to see and think about which one should be used a bit more aside from just the method name.


And really all of this could be refactored to just do an upsert on the API side vs explicit save and update, and maybe that's what we should do in the end, but at the moment I'm still feeling like having distinct routes for each. Whateva, we'll see which way the wind is blowing with me in the future ;P

Fixing a snag: graceful handling of trying to update a record that doesn't exist

A minor fix that's included in this PR is adjusting the update logic to gracefully handle the error that occurs when you try to update an animation that doesn't exist, i.e. you use an ID for a record that doesn't exist in the database.

I found this snag by accident when testing the code before pushing the commit, so I popped in and changed the ungraceful 500 that gets thrown with a graceful 422 (unprocessable entity) exception.

@chris-schmitz chris-schmitz linked an issue Jan 2, 2024 that may be closed by this pull request
@chris-schmitz chris-schmitz merged commit 2e149f1 into main Jan 2, 2024
1 check passed
@chris-schmitz chris-schmitz deleted the issue-update-mapping-to-require-id branch January 2, 2024 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update mapping to require id
1 participant