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

Refactor Core Functions for Clarity and Performance #6093

Closed
wants to merge 16 commits into from

Conversation

Ayoub-Mabrouk
Copy link

@Ayoub-Mabrouk Ayoub-Mabrouk commented Oct 28, 2024

This pull request includes several refactorings to improve code clarity, conciseness, and performance:

  • Refactored Functions:

    • req.is, app.engine, View.prototype.resolve, and tryStat for better readability.
  • Optimizations:

    • Used includes() in normalizeType.
    • Enhanced normalizeTypes with better variable declarations and array initialization.
  • Modern Syntax:

    • Simplified acceptParams and improved createETagGenerator.
  • Cleaner Logic:

    • Streamlined return logic in compileQueryParser.

These changes enhance maintainability while preserving functionality.

…dability

- Removed unnecessary variable `fn` and directly returned query parser functions
- Used template literals for error message for consistency with modern syntax
…ability

- Replaced multiple `if` conditions with `switch` for clearer type handling
- Simplified boolean function return and direct array handling in `proxyaddr.compile`
- Ensured support for empty/default cases with a default return
- Replaced function declaration with arrow function
- Separated ternary condition into multiple lines for clarity
- Changed `var` to `const` for better scoping and modern standards
- Improved variable declarations using const and let
- Simplified parameter parsing with destructuring
- Ensured the return structure remains consistent
- Use const for length and let for loop variable
- Initialize result array with specific length for better performance
- Maintain functionality while enhancing code readability
- Replaced indexOf() with includes() for clarity and improved readability.
- Maintained original functionality while streamlining the code.
- Removed unnecessary variable declaration for function assignment.
- Simplified return statements for false and valid cases.
- Improved error message formatting using template literals.
… handling

- Updated debug message to use template literals for improved readability.
- Removed the explicit error variable in the catch statement for cleaner syntax.
… conciseness

- Replaced var with const and let for variable declarations.
- Used optional chaining for file existence checks.
- Simplified logic by removing intermediate variable assignments.
- Added an explicit return of undefined if no valid file is found.
- Used const for variable declaration to enhance readability.
- Replaced conditional logic with startsWith() for cleaner extension validation.
- Added comments for clarity on functionality.
- Simplified argument handling by using Array.from() for flattened arguments.
- Used const for variable declaration to enhance readability.
@UlisesGascon
Copy link
Member

Hey @Ayoub-Mabrouk! Thanks for this PR, can you run npm run -- --fix and commit the changes? That will solve the CI issues :)

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

I think the main issue with this PR is that it does too much at once. I think we would likely accept a most of this if it was done in separate PRs with clear and small individual changes.

I would work on making sure each section of change is done in an individual PR and that PR has a clear description of why refactoring that section is helpful. Also, if we are going to discuss "for performance" we need some benchmarks. It is fine if we think it will improve perf in theory, and we might still land them because of that, but if that is the case I think we should just avoid calling it "perf" until we have those benchmarks.

@Ayoub-Mabrouk
Copy link
Author

hey @wesleytodd Thanks for the feedback! I’ll create separate PRs with smaller, targeted improvements, each with a clear description of the refactoring benefits.
For performance changes, I won’t label them as "perf" until we have benchmarks to support the claims.
Should I close this PR and open new ones, or keep it open and work from here? Let me know if there's anything else you’d like to see!

@UlisesGascon
Copy link
Member

Thanks for split this into multiple PRs ❤️ !

I added a reference on the description of each PR so now we can follow it better and enabled the CI too.

I will close this PR now 😉

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

Successfully merging this pull request may close these issues.

4 participants