nicknguyen-ccm / crudapp

Test Vue CRUD Application
0 stars 0 forks source link

Feedback #2

Open ethan-ccm opened 8 months ago

ethan-ccm commented 8 months ago

https://github.com/nicknguyen-ccm/crudapp/blob/0cd9edf61c6c895a1cd829b42a4fdf6b12774c85/src/components/HelloWorld.vue#L21 Moving this URL to a constant would be nicer than repeating it every time you need to call the API https://github.com/nicknguyen-ccm/crudapp/blob/0cd9edf61c6c895a1cd829b42a4fdf6b12774c85/src/components/HelloWorld.vue#L61-L63 All arrow functions with brackets that return on the first line can be converted to an arrow function without braces So instead of posts.value = posts.value.filter((item) => { return item._id !== id }) try posts.value = posts.value.filter((item) => item._id !== id) https://github.com/nicknguyen-ccm/crudapp/blob/0cd9edf61c6c895a1cd829b42a4fdf6b12774c85/src/components/HelloWorld.vue#L75 This computed is a little cluttered and has some duplicate code so it can likely be simplified quite a bit. Try thinking of how to approach this differently to only use one call of filter / sort or maybe break it into multiple computed's. Additionally you should take a look at this method https://github.com/nicknguyen-ccm/crudapp/blob/0cd9edf61c6c895a1cd829b42a4fdf6b12774c85/src/components/HelloWorld.vue#L70 This might be a bit of a nitpick but a better name might be setSortMode though this also doesn't necessarily need to be in a function. Just a matter of preference.

nicknguyen-ccm commented 8 months ago

Thanks for the feedback!

would you mind reviewing my code again once I finish Sean's assignment and refactor this code?

ethan-ccm commented 8 months ago

@nicknguyen-ccm Yeah, no problem. And please let me know if you disagree with any of what I said or think I could have clarified something better