Sofiia assignment week 2 #21
Conversation
| ); | ||
| const data = await response.json(); | ||
|
|
||
| if (data.cod === "404") { |
There was a problem hiding this comment.
This isn't blocking for the review, but something you can think about - what would happen if you get a different error code from the weather service? What would happen in your code and are you ok with that?
There was a problem hiding this comment.
yes makes sense. i've changed it for (data.cod !== 200). thank you!
There was a problem hiding this comment.
Oh I wasn't necessarily saying you should change it, just was pointing out that situation is something to think about :)
| if (data.cod !== 200) { | ||
| return res.status(data.cod).json({ weatherText: "City is not found!" }); | ||
| } |
There was a problem hiding this comment.
I like that you're thinking about other error situations but this code actually can create a confusing scenario for the end-user. Take this example:
- Your API key is invalid so when your backend calls the open weather API you receive a 401 code. (You can test this by adding some extra characters to your api key)
- Your weather API will then send the 401 back to the user and say "City is not found!". This is potentially confusing because 1) the error message doesn't match the code (401) and 2) from the perspective of the user they did nothing wrong, so it won't be clear why they received a 401.
How might you change the code to correctly explicitly handle both a 404 as well as any other error?
There was a problem hiding this comment.
Yes, I completely agree it would be weird to receive a message "City is not found" with error 401. So, I've changed it with a 'double' condition:
if (data.cod !== 200) { if (data.cod === "404") { return res.status(data.cod).json({ weatherText: "City is not found!" }); } return res.status(data.cod).json({ weatherText:${data.message}}); }
So now, if we have an error, we will see the correct message about this specific error.
There was a problem hiding this comment.
The double condition is a good way to go, and you could even consider using a switch statement to handle the different error codes. Looks good!
In a real-life situation you might want to be careful about passing error messages directly from the open weather api to your caller (i.e. weatherText: ${data.message}) because that can be potentially confusing if the caller of your API doesn't know what the backend is doing under-the-hood. You could even risk exposing something internal that you don't want to expose. What action you take for these "other" errors will depend on who your caller is, whether your API is public/private, your business use-cases, etc. Of course for this exercise we don't need to worry about all that :)
No description provided.