DARYNA_TKACHENKO-w2-Node.js #13
Conversation
durw4rd
left a comment
durw4rd
left a comment
There was a problem hiding this comment.
Good job, Daryna! You've used several coding patterns that went beyond what was asked in the instructions and made your application more robust; great that you explore these concepts even when not explicitly required in the exercise instructions!
There was a problem hiding this comment.
I know the project instructions don't mention this, but as a note for future projects, avoid committing API secrets to GitHub repositories. It's better to store them as environment variables, for example.
| }); | ||
|
|
||
| app.post("/weather", async (req, res) => { | ||
| const { cityName } = req.body ?? {}; |
There was a problem hiding this comment.
Using the nullish coalescing is a great practice for defensive programming, but note that the express.json() middleware, the res.body will always be at least an empty object, so it's not necessary here.
|
|
||
| app.post("/weather", async (req, res) => { | ||
| const { cityName } = req.body ?? {}; | ||
| if (!cityName || typeof cityName !== "string" || !cityName.trim()) { |
There was a problem hiding this comment.
Well done with the thorough input validation!
| return res.status(400).json({ weatherText: "Please provide a cityName." }); | ||
| } | ||
|
|
||
| const url = `https://api.openweathermap.org/data/2.5/weather?q=${encodeURIComponent( |
There was a problem hiding this comment.
Great that you are using encodeURIComponent to properly encode the city name in the URL!
| )}&units=metric&appid=${keys.API_KEY}`; | ||
|
|
||
| try { | ||
| const r = await fetch(url); |
There was a problem hiding this comment.
Nit: A more descriptive name of the variable would be handy.
| const r = await fetch(url); | ||
| const data = await r.json(); | ||
|
|
||
| if (!r.ok || String(data.cod) !== "200") { |
There was a problem hiding this comment.
Great that you're checking the value of the data.cod value; I don't think I've seen anyone do this before!
I'm not that familiar with the API, but it seems a bit of a leap from the code not being '200' to concluding that it's a 404, city not found response, though. A more robust approach would be reading the cod value and returning the actual code the API provided.
| const name = data?.name ?? cityName; | ||
| const temp = data?.main?.temp; |
There was a problem hiding this comment.
Good use of the optional chaining for safe property access 👍
| const request = supertest(app); | ||
|
|
||
| describe("POST /weather", () => { | ||
| it("Quick test", () => expect(1).toBe(1)); |
There was a problem hiding this comment.
With the real tests added, this placeholder could be removed to reduce the noise.
|
Thank you very much! |
2 participants
No description provided.