-
Notifications
You must be signed in to change notification settings - Fork 6
Assignments week 1 Nikita #6
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?
Conversation
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.
Great work on your first assignment, everything fits the criteria and worked as expected.
The feedback I have left is just some advice on readability. I would recommend checking out and fixing the layouts/views comment about handlebars templates to understand how to use them better.
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 have put the entire page in the "layout" section here. This works for one page, but if we had more, we could not reuse this layout system.
Layouts:
-> Defines the structure of page(s).
views:
-> Defines the specific content in the page.
For this example,
- We would place "<h1>{{{body}}}</h1>" in the layouts/main.handlebars.
- We would place "Hello from the backend to the frontend" in the home.handlebars file
This would mean we can reuse the layout if we had more than one view.
res.render('home') | ||
}) | ||
|
||
app.use(express.json()) |
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.
For readability, we group functions together where possible.
In this case, put all middlewares before defining the routes. This makes it easier to find all the middlewares included in the program (and the order that they run in!)
app.use(express.json()) | ||
|
||
app.post('/weather', (req, res) => { | ||
const cityName = req.body.cityName |
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.
It's good practice to check if a value exists when you aren't sure, in this example if the request does not have a body value (you can test this in Postman by setting the body to null https://gyazo.com/c075d8df776a091bc53bfa01ec459b80), then the error is returned to the user unhandled.
To improve this we can check if req.body?.cityName == undefined
then return a clear error saying "city not found" (check res.status() for info on how to return an error status).
No description provided.