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: Improve Code Readability with Constants and Early Returns #842

Closed
wants to merge 4 commits into from

Conversation

k-vanio
Copy link

@k-vanio k-vanio commented Aug 13, 2023

Refactor: Improve Code Readability with Constants and Early Returns

This commit refactors the realIP function by introducing constants for header names
(trueClientIP, xRealIP, xForwardedFor) to enhance code clarity. Additionally, it
utilizes the concept of early returns for simplifying the logic and improving overall
readability.

These changes contribute to better maintainability and understanding of the codebase.

@lesichkovm
Copy link

@k-vanio, you have done good job, two points:

  1. The constants should be alphabetically sorted to improve readability
  2. You have missed the net.ParseIP(ip) logic which was checking the IP for validity

@k-vanio
Copy link
Author

k-vanio commented Aug 28, 2023

@lesichkovm Thank you for your feedback! I've addressed the first point by alphabetically sorting the constants. However, regarding the second point about the net.ParseIP(ip) logic, I couldn't identify any issues with it as it seems correct to me. Could you please provide more details or context about what you've observed?

@lesichkovm
Copy link

Did not see the new function isValidIP. All is good @k-vanio

middleware/realip.go Outdated Show resolved Hide resolved
…alidate the integrity of IP addresses within the X-Forwarded-For (XFF) header.
@k-vanio k-vanio requested a review from VojtechVitek August 30, 2023 12:55
middleware/realip.go Outdated Show resolved Hide resolved
@k-vanio k-vanio requested a review from VojtechVitek August 30, 2023 14:36
Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

LGTM

@PauloRobertTlss
Copy link

LGTM 🚀

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

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

Hi @k-vanio,

Thanks for the Pull Request and sorry it did't get merged to the master branch.

I decided to merge the following PR instead, which adds a new feature and also improves modularity and readability at the same time:

Superseded by #908.

Best,
Vojtech

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

Successfully merging this pull request may close these issues.

4 participants