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

Improve S3398: Fix code smells #6827

Merged
merged 2 commits into from Mar 1, 2023

Conversation

zsolt-kolbay-sonarsource
Copy link
Contributor

Fixes #6706
RSPEC PR

Copy link
Contributor

@andrei-epure-sonarsource andrei-epure-sonarsource left a comment

Choose a reason for hiding this comment

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

I don't find this more readable.

Can you find something that uses more positive logic and less indirection?

}

return firstPath[lastCommonPathIndex];
bool NotEveryNodeIsTheSameOnLevel(int level) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find this clearer because it's two negations:

  • NotEveryNodeIsTheSameOnLevel
  • != inside Any

Also, levels refers to indeces, whereas NotEveryNodeIsTheSameOnLevel is iterating over typeHierarchyFromTopToBottom

But NotEveryNodeIsTheSameOnLevel is called on levels

There's a lot of indirection in here. levels.Do(Action) actually iterates on a different data structure, using the indeces inside levels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted it. Now the only change is splitting the comment to multiple lines.

Comment on lines 111 to 112
? minPathLength - 1
: firstDifferingLevel - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

having -1 makes it a bit difficult to follow the logic...

there's another implicit negation here: Differ rather then Common in the name of the method: LowestCommonAncestorOrSelf

So the method is named Common, but the logic inside is looking for Different

@sonarcloud
Copy link

sonarcloud bot commented Mar 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Mar 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@andrei-epure-sonarsource andrei-epure-sonarsource merged commit cc3e1fd into master Mar 1, 2023
@andrei-epure-sonarsource andrei-epure-sonarsource deleted the Zsolt/S3398-code-smells branch March 1, 2023 13:15
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.

New Rule S3398: "private" methods called only by inner classes should be moved to those classes
2 participants