barcabarca216 / MI-449-SS17-741-js-server-intro-jtrO3q

0 stars 0 forks source link

Project Feedback #1

Open barcabarca216 opened 7 years ago

barcabarca216 commented 7 years ago

@egillespie Can you take a look at this? It's hosted here and meets the following criteria:

egillespie commented 7 years ago

LOL That's a great error page! Overall, the project is really good too. I do have one suggestion to make your code easier to read, and there's one criteria you missed:

Splitting up strings

To make your code easier to read (i.e. to avoid having to scroll horizontally a lot), try breaking up your strings like this:

response.end(
  '<h1>HOME</h1>'
  + '<p>HOW ARE YOU THIS FINE DAY?</p>'
  + '<a href="/cuteness">Click here for adorable animals</a>'
  + ' <a href="/random-joke">Click here for hilarious jokes</a>'
)

Error page does not show bad URL

Your error page literally made me laugh out loud, but it doesn't show the requested URL per the second-to-last criteria. Do you mind adding that in?


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! :rocket:

barcabarca216 commented 7 years ago

Not sure if the requested URL part is correct but it is updated.

egillespie commented 7 years ago

Hi @barcabarca216, one thing I would recommend is to double-back to the Use JavaScript to collect, remember, and display info lesson and follow the steps on the first page to install the JavaScript linter. The indenting of your if/else if/else blocks is misaligned and makes it difficult to catch issues in your code.

As for showing the URL on the error page, here are some suggestions:

There are no comparisons in else

I see the following line:

else (request.url = '/lostloveandregrets')

I think you should remove everything after else on that line. if/else tests should look like this:

if (a == b) {
  // ...
} else {
  // ...
}

Add request.url to the error response

If I visit the page /bad-url, I would actually like to see something like this on the error page:

This is not the page you are looking for: /bad-url

Notice how the value of request.url is added to the end of the message.


Give these a shot. If you have questions, hit me up in class and I'll work through them with you.

Good luck! ☘️

barcabarca216 commented 7 years ago

I had the js-linter installed but breaking up the strings caused a lot of errors and realignments. I added the url value to the error page. Let me know if I am making any progress.

egillespie commented 7 years ago

Yeah, now you're cooking with fire! 🔥 :shipit: