Skip to content

improve code readability#3446

Merged
ndyakov merged 1 commit into
redis:masterfrom
cxljs:improve-code-readability
Aug 4, 2025
Merged

improve code readability#3446
ndyakov merged 1 commit into
redis:masterfrom
cxljs:improve-code-readability

Conversation

@cxljs

@cxljs cxljs commented Jul 25, 2025

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

ndyakov commented Jul 25, 2025

Copy link
Copy Markdown
Member

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

cxljs commented Jul 25, 2025

Copy link
Copy Markdown
Contributor Author

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

ndyakov commented Jul 25, 2025

Copy link
Copy Markdown
Member

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-oss

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

ndyakov commented Jul 30, 2025

Copy link
Copy Markdown
Member

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

cxljs commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

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 <fukua95@gmail.com>
@cxljs cxljs force-pushed the improve-code-readability branch from 0be47f1 to a7fb1aa Compare August 2, 2025 08:51

@ndyakov ndyakov left a comment

Copy link
Copy Markdown
Member

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
ofekshenawa pushed a commit that referenced this pull request Aug 10, 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`.

Signed-off-by: Xiaolong Chen <fukua95@gmail.com>
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