Yaroslav kazeev w2 node js #8
Conversation
durw4rd
left a comment
durw4rd
left a comment
There was a problem hiding this comment.
Thanks for your work Yaroslav - it meets the requirements of the assignment, and it's clear you know how to create an Express server, including testing.
With that said, I'd recommend looking again into the response you get back from the API, specifically into the units the API uses and a way to control this. As it stands, your server is serving back questionable data.
| // Add middleware to parse JSON data | ||
| const data = await response.json(); | ||
| if (response.ok) { | ||
| const temperature = data.main.temp / 10; |
There was a problem hiding this comment.
The API provides the temperature in different units depending on the instructions given via the request query params. The way you parse the response here will give a number that may seem accurate under certain scenarios, but it certainly won't be a correct temperature in the selected destination ;)
There was a problem hiding this comment.
Yeah, my bad, thanks. Kelvin degrees can be converted into Celsius by subtracting 273,15. That is another formula with Fahrenheit conversion, where there is division by a constant.
| } else { | ||
| throw new Error(`Error: ${data.message}, Status: ${data.cod}`); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
It would be cleaner and likely more helpful with potential debugging if you had separate handling for a missing cityName vs. an incorrect API key (or other reasons for error).
There was a problem hiding this comment.
This line of code allows exactly what you've suggested for different scenarios:
Error: Invalid API key. Please see https://openweathermap.org/faq#error401 for more info., Status: 401
Error: city not found, Status: 404
app.js:26
While a generic response is sent to the user:
res
.status(404)
.json({ error: "City is not found or the API key is wrong" });
This approach was suggested to us by another teacher, Utku. He was concerned about security and did not want to send specific error messages to the client, as this could open the way for a hacker to attack. Instead, specific messages should be logged and analyzed on the server side, which is what is done here.
|
Michal, thanks for your review. It never hurts to have an additional pair of eyes to be sure that everything is correct. |
2 participants
No description provided.