Storage assignment

[folkertjan]

Storage Assignment

link to repo

Thoughts

This assignment took a lot less time than the Shelter assignment. Probably the part where I was most unsure of what I was doing was setting up the SQL database. After that, rewriting the express server to get data from the SQL database instead of the local db folder was not too difficult. (It's working at least) If I was supposed to give different error messages back, please let me know as I am not really certain I found ways to deliver all of them 😅 Furthermore I wasn't sure what's a better practice: changing the query to exclude unused data fields or decide within the template pages to leave certain data object properties away. P.S. also not sure if the package.json file is still correct as I have no clue what half of the scripts / test packages do.

Imma get some well earned rest now

[wooorm]

This looks great, and thanks for the cat! 😸

Review

• I had to look at the code to populate the .env file, I suggest adding an example in the readme, like so
• For branches, I suggest keeping master at the “most recent” stuff. You could use a shelter branch for the code as it was back at the shelter assignment, storage for how it was when you handed it in, and master as the most recent stuff (so same as storage in this case). That makes sense to me at least! 🤷‍♂️
• the diagram looks great!
• I suggest using id in every table as the primary key, makes more sense than sh_id
• great addition for the shelter table, and I personally prefer shelterId over shelterID (as it corresponds with most stuff in JavaScript, like document.getElementById)!
• is next still needed after showErrorPage?
• when an animal is added, and when it fails, you try to remove the file, then render a template, and finally the result of removing the file comes back (but you’ve already sent an error page back)
• always handle errors, right now fs.rename could fail with an error, but it’s not logged anywhere, so you’d never know! Now I get (node:35800) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated.
• I’d suggest checking if the id is valid/invalid before connecting with the database!
• is notFound still used? Should it send something back?
• For a future addition, you could render the form dynamically! Right now, the shelters are hard coded in view/form, but you could also SELECT all animal shelters and pass that to the form and render the <option>s dynamically!
• Do you still need to send data back with a 204?
• Why not SELECT * FROM animal LEFT JOIN shelter ON animal.shelterId=shelter.sh_id WHERE animal.id = ?
• Is weight an int?
• Is there maybe a specific data type for dates?

Feedback

This assignment took a lot less time than the Shelter assignment. Probably the part where I was most unsure of what I was doing was setting up the SQL database.

How much time did both parts take?

If I was supposed to give different error messages back, please let me know as I am not really certain I found ways to deliver all of them 😅

But other than those it looks rather good!

Furthermore I wasn't sure what's a better practice: changing the query to exclude unused data fields or decide within the template pages to leave certain data object properties away.

That really depends. I think for the list route it makes sense to just SELECT the properties that are needed, as with many animals it could be significantly faster. The detail page would result in a less significant improvement. But anyway, this only really starts to matter in production and with many people using the site!

P.S. also not sure if the package.json file is still correct as I have no clue what half of the scripts / test packages do.

Looks good! There’s some dev-dependencies, build.js, and tests still left over from the prev db. Then there’s some stuff to check code-style, (stylelint, xo, remark), but you’re free not to use them!

All in all, well done!