Skip to content

Assignments week 1 Ahmad Zetouny #3

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Zetouny
Copy link

@Zetouny Zetouny commented Apr 28, 2025

No description provided.

@Zetouny Zetouny changed the title Ahmad_Zetouny-w1-Node Assignmets week 1 Ahmad Zetouny Apr 28, 2025
@Zetouny Zetouny changed the title Assignmets week 1 Ahmad Zetouny Assignments week 1 Ahmad Zetouny Apr 28, 2025
Copy link

@Pandelissym Pandelissym left a 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! All the requirements from the assignment are met.

I have left a few suggestions for improvements, minor things.

import express from "express";

const app = express();
const port = 3000;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constants like this are usually written in all uppercase (PORT instead of port). This is a convention that improves readability.

});

app.post("/weather", (req, res) => {
const cityName = req.body.cityName;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion here (non-blocking) would be to log the city name before returning it. This allows you to easily debug/see what is going on when your /weather endpoint is called. You can log whatever you want (e.g. City name: cityName or the whole body object).

@Zetouny
Copy link
Author

Zetouny commented May 5, 2025

Thank you @Pandelissym, for your time.
I've made the required changes as advised.

@Pandelissym
Copy link

Thank you @Pandelissym, for your time.

I've made the required changes as advised.

I would only make port uppercase. By convention only string literals or numbers that wont be changed but are just going to stay constant at that value (only read) are uppercased. The app variable should still be lowercase 🙂

@Zetouny
Copy link
Author

Zetouny commented May 5, 2025

My bad, I thought you meant that variables being in the global scope should always be uppercase. 😅

@Pandelissym
Copy link

My bad, I thought you meant that variables being in the global scope should always be uppercase. 😅

No worries! Looks goot me now. Approved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants