Assignments week 2 YANA SENIUK #9
Conversation
…de.js-Cohort52 into YANA_SENIUK-w2-Node
There was a problem hiding this comment.
Great job! All requirements of the assignment are met! Really great work on the project structure, you have split the concepts up correctly and added nice helper functions! Also props on the nice error handling, very clean!
I have added some suggestions including your .env file and a comment on your test suite!
| @@ -0,0 +1 @@ | |||
| PORT=3000 | |||
There was a problem hiding this comment.
Nice job on using a env file! However your .env should never be pushed to git!
| @@ -0,0 +1 @@ | |||
| export const API_WEATHER_KEY = "007dd5ca6a8f91773d26bde4f0099937"; | |||
There was a problem hiding this comment.
Since you are using env files, that would be a perfect place to store your API key! Putting your key on github is dangerous as it can be leaked leading to unauthorized access.
| @@ -0,0 +1,6 @@ | |||
| export function errorHandler(err, req, res, next) { | |||
There was a problem hiding this comment.
Really nice job on using a custom error handler!
| @@ -0,0 +1,9 @@ | |||
| export function validateBody(req, res, next) { | |||
There was a problem hiding this comment.
I would change the name of this file to match the others or change the others to match this one! It doesnt really matter which one as long as you are consistent! I would personally not add .middleware in the file name as they are already in a middleware folder.
| export async function getCityCurrentWeather(req, res, next) { | ||
| const { cityName } = req.body; | ||
|
|
||
| if (!cityName?.trim()) { |
There was a problem hiding this comment.
You dont need to trim before you check if cityName exists!
|
|
||
| try { | ||
| const { name, main } = await fetchCurrentWeather(cityName); | ||
| res.status(200).json({ cityName: name, temperature: main.temp }); |
There was a problem hiding this comment.
Before returning, you should validate what your api returned. I would check if main.temp and name is defined before sending it!
| }); | ||
|
|
||
| describe("POST /api/weather", () => { | ||
| it("Should response JSON with cityName and temperature properties", async () => { |
There was a problem hiding this comment.
Very good tests! I would also add a test/extended your happy flow test to check the response data. e.g.g cityName should match the city passed to the api call and for temperature (since of course you cant know what to expect) you can check the type!
No description provided.