Skip to content

Assignments week 1 YANA SENIUK #5

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 8 commits into
base: main
Choose a base branch
from

Conversation

yanaesher
Copy link

No description provided.

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.

Very good work on the first assignment! All criteria are met and you want beyond the expectations! Good job on separating your logic into folders and modules, adding a gitignore, using environmental variables and using error handlers and middleware!

I have left a few suggestions for improvements that you can take a look at.

const app = express();
const PORT = process.env.PORT || 3000;

function main() {

Choose a reason for hiding this comment

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

There is no need to create a function for this and call it. You can remove the function and the call to main(). This will make your code more readable!

Choose a reason for hiding this comment

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

This file should also be inside src as it is part of your code!

//non-exist path
app.use(notFound);

//error handling

Choose a reason for hiding this comment

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

These comments are not necessary. Good job and trying to make the code readable but you already use very clear variable names! The comments are not adding much as it is already clear from reading the code

"main": "server.js",
"type": "module",
"scripts": {
"start": "node --watch --env-file=.env server.js"

Choose a reason for hiding this comment

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

A tip here would be to create two scripts. One called dev and one called start. The dev script could then use the --watch flag and the start script without the watch flag. This seperates development from production code! You dont want to be using --watch in production as it is less performant and more dangerous!

Choose a reason for hiding this comment

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

Great job on adding a gitignore!

});

//Api Routes
app.use("/api/weather", weatherRouter);

Choose a reason for hiding this comment

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

Adding the /api/ path is a very good practice good job! I would also move the GET / route under the /api/ path to keep things consistent as that route is also part of your api!

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.

Looks good!

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