-
Notifications
You must be signed in to change notification settings - Fork 11
Hossein-w2-node.js #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Hossein-w2-node.js #14
Conversation
Hossein-Kelisa
commented
Feb 24, 2025
- Connected OpenWeatherMap API to fetch weather data.
- Added error handling for missing or invalid city names.
- Wrote tests to make sure everything works correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very well done, Hossein! You did a great job on this assignment.
I left some comments for improvement; please have a look.
There is another thing needs to be fixed; please move the files from
assignments/config-files/hackyourtemperature
to assignments/hackyourtemperature
.
expect(response.status).toBe(200); | ||
expect(response.body.weatherText).toContain('Amsterdam'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This unit-test is great, and it covers the main cases. One minor improvement to enhance readability and maintainability is to consider standardizing the code style; ensure consistent spacing, method chaining, and line breaks.
For example, line 8:
const response = await request.post('/weather').send({});
All the calls in one line, but in line 16:
const response = await request
.post('/weather')
.send({ cityName: 'aslkdjflaskjd' });
It's in multiple lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right! I'll standardize the code for better readability and maintainability. Thanks for the tip
} | ||
}); | ||
|
||
export { app }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is good and functional, but the format can be improved. You could use the built-in functionality in VS Code to format the code https://code.visualstudio.com/docs/editor/codebasics#_formatting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion!
@@ -0,0 +1,5 @@ | |||
const keys = { | |||
API_KEY: "d19e6d84be4dc190db7e1381f4ade935" | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is in the instructions https://github.com/HackYourAssignment/Node.js-Cohort51/blob/main/week2/MAKEME.md#311-setting-up-the-api that you have to put the key in the file. But for enhanced security, it's best practice to use environment variables. The dotenv
library (https://github.com/motdotla/dotenv) facilitates this by enabling you to store sensitive information, like your API key, in a .env
file, which is then loaded into the environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the security tip! I'll definitely use dotenv to store the API key in a .env file
Hi @eidosam I have resolved all your suggestions.Please review the updates and let me know if anything else is needed. Thanks for your feedback and for your time, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Hossein-Kelisa, for addressing the feedback! But there are still issues with the code format; could you please fix it? This document might help: https://code.visualstudio.com/docs/editor/codebasics#_formatting.
Also, the dotenv
is missing from the dependencies in package.json
.
Hi @eidosam |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks, @Hossein-Kelisa!