Skip to content
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

Update code #133

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

Update code #133

wants to merge 3 commits into from

Conversation

Sup3820
Copy link

@Sup3820 Sup3820 commented Mar 5, 2023

Small updates to improve code, fix small bugs, better readability

update code and optimize and fix small bugs
improved code, optimized and shrunk by 10%
@Prince25 Prince25 added enhancement New feature or request good first issue Good for newcomers labels Mar 6, 2023
@Prince25 Prince25 self-assigned this Mar 6, 2023
@Prince25
Copy link
Owner

Prince25 commented Mar 6, 2023

I browsed the changes briefly but please do me a favor. Could you run npm run prettierAll and then npm run eslintFixAll? This will make sure the formatting matches and will make it much easier for me to see the changes.

Thanks!

image: undefined,
title: null,
inventory: null,
image: null,
Copy link
Owner

Choose a reason for hiding this comment

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

Please tell me whether this makes any difference or the reason this was changed.

@Prince25
Copy link
Owner

Prince25 commented Mar 7, 2023

The current code changes seem to be rewritten for conciseness, no? If there's any bug fixes, can you point out where they are?

From what I've gathered:

  • require changed to import
  • moved imports and constants to the top
  • took out some console statements
  • used arrow functions inside app.get
  • changed undefined to null
  • getPage() revision
  • extractInformation() revision

While I totally agree your changes do make the code easier to read and follow, there are reasons for structuring/writing it the way I did. I wanted to be clear about what the code does and what the program is doing when launched (for debugging and newbie purposes). It didn't make sense to me to import all modules if an environment file is not found. As for import vs require, I usually prefer import but there are good reasons to use require.

However, I will incorporate some of your changes like using arrow functions inside app.get, app.listen being at the end, and perhaps getPage and extractInformation function changes once I test them out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants