Skip to content

improve code readability #3446

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

Merged
merged 1 commit into from
Aug 4, 2025
Merged

Conversation

cxljs
Copy link
Contributor

@cxljs cxljs commented Jul 25, 2025

  • replace two similar functions appendUniqueNode and appendIfNotExists with a generic function.

  • simplify the implementation of the get method in clusterNodes.

  • keep the member name _generation of clusterNodes consistent with other types.

  • rename a data member _masterAddr to masterAddr.

@ndyakov
Copy link
Member

ndyakov commented Jul 25, 2025

Hey @cxljs, thanks for your contribution.
Do you have a specific reason to prefer less linebrakes. I do think, sometimes multiline function declaration is better for readibility?
Those function declarations and the generic function for append are the only one that I am not so happy about including, but I see the benefit of having that generic...

@cxljs
Copy link
Contributor Author

cxljs commented Jul 25, 2025

Hello @ndyakov , the reason for reducing line breaks is that there will be a comma after the last parameter, which often makes me think there are more parameters on the next line. e.g.:

func masterReplicaDialer(
	failover *sentinelFailover,
) func(ctx context.Context, network, addr string) (net.Conn, error) {

But this is just my personal opinion, if you think multiline function declaration is better for readibility, let's keep it as it is.

@ndyakov
Copy link
Member

ndyakov commented Jul 25, 2025

I do have a preference, but I am not the only person working on this :) @htemelski-redis , @ofekshenawa feel free to share your opinions.

@htemelski-redis
Copy link
Contributor

I am a bit on the fence about the function definitions.
I try to keep my lines shorter than 110-120 characters, and the proposed change is consistent with that.
On the other hand, for functions that accept more than a few arguments I preffer to separate them into new lines each

func (c cmdable) GeoRadiusByMember(
    ctx context.Context, 
    key, member string, 
    query *GeoRadiusQuery,
) *GeoLocationCmd {

@ndyakov
Copy link
Member

ndyakov commented Jul 30, 2025

I also do like what @htemelski-redis is suggesting.

@cxljs cxljs force-pushed the improve-code-readability branch 2 times, most recently from 3bfb847 to eb8df22 Compare July 31, 2025 07:49
@cxljs
Copy link
Contributor Author

cxljs commented Jul 31, 2025

I've reverted the function definitions to their original format.

@cxljs cxljs force-pushed the improve-code-readability branch from eb8df22 to 0be47f1 Compare August 1, 2025 06:21
- replace two similar functions `appendUniqueNode` and `appendIfNotExists` with a generic function.

- simplify the implementation of the `get` method in `clusterNodes`

- keep the member name `_generation` of `clusterNodes` consistent with other types.

- rename a data member `_masterAddr` to `masterAddr`.

Signed-off-by: Xiaolong Chen <[email protected]>
@cxljs cxljs force-pushed the improve-code-readability branch from 0be47f1 to a7fb1aa Compare August 2, 2025 08:51
Copy link
Member

@ndyakov ndyakov left a comment

Choose a reason for hiding this comment

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

@cxljs Thank you for the contribution! This looks good.

@ndyakov ndyakov merged commit 375fa5d into redis:master Aug 4, 2025
20 checks passed
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.

3 participants