Skip to content

Refactor: Improve Code Readability with Constants and Early Returns#842

Closed
k-vanio wants to merge 4 commits into
go-chi:masterfrom
k-vanio:master
Closed

Refactor: Improve Code Readability with Constants and Early Returns#842
k-vanio wants to merge 4 commits into
go-chi:masterfrom
k-vanio:master

Conversation

@k-vanio
Copy link
Copy Markdown

@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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

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

Comment thread middleware/realip.go Outdated
…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
Comment thread middleware/realip.go Outdated
@k-vanio k-vanio requested a review from VojtechVitek August 30, 2023 14:36
Copy link
Copy Markdown
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
Copy Markdown

LGTM 🚀

Copy link
Copy Markdown
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