Assignments week 2 Nikita #12
Conversation
|
|
||
| describe('POST /', () => { | ||
| it('returns temperature for a valid city', async () => { | ||
| const fakeGeoData = [{ lat: 52.37, lon: 4.89 }] |
| try { | ||
| const cityName = req.body.cityName | ||
| const geoData = await fetch( | ||
| `http://api.openweathermap.org/geo/1.0/direct?q=${cityName}&limit=1&appid=${API_KEY}` |
There was a problem hiding this comment.
Whenever you have to write domains multiple times, it's good to turn them into variables/constants so they can be switched out, and so you can make sure theirs no typos in the project. e.g.
openWeathMapApi = "http://api.openweathermap.org"
| const geoResponse = await geoData.json() | ||
| console.log(geoResponse) | ||
| if (geoResponse.length == 0) { | ||
| throw new Error('City is not found') |
There was a problem hiding this comment.
It is best not to use errors in "normal" functionality (a user typing a city that doesn't exist is usual behaviour). it is best to just write here:
res.status(404);
res.send({ weatherText: "City is not found!" });
return;
as this would also allow us to set specific messages/status based on the errors.
There was a problem hiding this comment.
Got it! Makes sense
I’ll change it
| temperature: (weatherResponse.main.temp - 273.15).toFixed(1) | ||
| }); | ||
| } catch (err) { | ||
| res.json(err.message) |
There was a problem hiding this comment.
Best to null check this, as in theory you can "throw" anything, (even though it should be an Error by convention).
e.g the line "throw 0;" is valid, even if silly/should not be used.
Therfore, we should update this to res.json(err?.message ?? 'unknownError'). incase of null/undefined messages.
Also, when we return an error, it's good to set a different status (i.e. not 2XX) response codes, https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status A full list can be found here, but the common ones are 404 - not found, 403 - forbidden/ not logged in, 500 - server error, so 500 is a good catch all for unexpected errors like this.
There was a problem hiding this comment.
Thnx, Edward! I’ll add the null check and set the status to 500 for unknown errors
| @@ -0,0 +1 @@ | |||
| export const API_KEY = '7d47d5f41dcdf3b026655cb15bf3ff0a' | |||
There was a problem hiding this comment.
It's good practice to not push up API keys to repo's to keep them safe. two ways to do this are:
-> Ignoring this file in the .gitignore.
-> adding a .env file (and ignoring it) - This is better, as the code will still compile, and the .env file is a standard across a lot of software/programs, so loading them into the program at runtime is handleded well.
There was a problem hiding this comment.
You're totally right — I’ll move the key to a .env file and add it to .gitignore.
even github itself was not satisfied with my pullrequest -_-
No description provided.