search

FanZhiheng / MI-449-js-create-an-api-with-express

0 stars 0 forks source link

Project Feedback #1

Open FanZhiheng opened 5 years ago

FanZhiheng commented 5 years ago

@egillespie Hello Eric! My name is FanZhiheng.

This project is finished. Please check!

 I'm always the one who left earliest during your lab and I'm very sorry for leaving early every time.
 I'm a computer science major senior student and I have too many CSE classes taking this semester(this is my last semester in MSU). I always start your project at around 11:40 am before taking your lab so that I can finish your project before 1:30 pm and then I could leave early to go to my CSE project meeting.
 After I finished this project, my grade is going to hit 4.0. I think I'll be focusing on my CSE projects for the rest of the semester and I won't come to the lab anymore. I'm sorry :(.
egillespie commented 5 years ago

Hi @FanZhiheng, thank you for the transparency. You don't need to apologize — I've seen that you've been putting in the work and managing your time well.

Time management is probably one of the hardest and most important parts of MI 449 and it's something you've been doing really well. I think that will reward you well in your career! 🙂

I also completely understand that you want to put more effort into your remaining classes where you may not have as much flexibility in the how and when you do classwork. You've also shown great initiative in learning and using the skills covered in MI 449 and I'm confident you'll continue doing so once you're done with the class.

That being said, don't be stranger! If you want me to review a resume, cover letter, job application, or other projects, feel free to send them my way. You're also more than welcome to use me as a referral if you're applying to jobs. 😉

Okay, now time for your code review! You're off to a great start here and I see you're using lots of recommendations from the lesson, too. 👍

Here are some suggestions to improve your API. Can you try these out?

Remove ejs from package.json

It looks like ejs has been added as a dependency in package.json, but isn't used in your code. This won't cause harm in your code, but it will cause Heroku to download the unused dependency, which will lead to longer deployments.

Would you mind removing ejs from package.json?

I see that you are using npm instead of yarn, which is okay. To remove a package and uninstall the dependency you can run the command npm uninstall ejs.

Send JSON even in 404 responses

You have a line sending a 404 response in your code:

response.status(404).end('sorry, no such product: ' + request.params.id)

Would you mind changing this to send JSON? A good format for errors will send an object with a message, like this:

response.json({ message: request.params.id + ' not found' })

You may also want to replace the word "product" with "todo". 😉

After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! 🤘

FanZhiheng commented 5 years ago

@egillespie Problems fixed! Please check!

egillespie commented 5 years ago

Woot, your changes look and work great. 🎉 :shipit:

Congratulations on your 4.0! 🤘🤘🤘