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

minor: Fix sonarcube warnings #736

Merged
merged 2 commits into from
Aug 21, 2023
Merged

minor: Fix sonarcube warnings #736

merged 2 commits into from
Aug 21, 2023

Conversation

wajda
Copy link
Contributor

@wajda wajda commented Aug 16, 2023

Fix SonarCloud warnings

  1. Add missing comments for empty methods
  2. Remove unused definitions and commented out code
  3. Simplify boolean expressions
  4. Mute false security alarms on leaked secrets (dummy passwords used in unit tests for testing password processing)
  5. Mute code duplication warnings in tests

@wajda wajda marked this pull request as ready for review August 16, 2023 09:06
@wajda wajda requested a review from cerveada August 16, 2023 09:06
@github-actions
Copy link

JaCoCo code coverage report

There is no coverage information present for the Files changed

Total Project Coverage 42.11% 🍏

Comment on lines 41 to 48
if (attrIfd == id) true
else dependsOnRec(attrMap(attrIfd).childRefs, id)
attrIfd == id || dependsOnRec(attrMap(attrIfd).childRefs, id)
case ExprRef(exprId) =>
if (exprId == id) true
else {
if (litMap.contains("exprId")) false
else dependsOnRec(funMap(exprId).childRefs, id)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's shorter, but for me, the new version is harder to understand.

Copy link
Contributor Author

@wajda wajda Aug 17, 2023

Choose a reason for hiding this comment

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

What about introducing an intermediate val? For example?

lazy val attrChildIds = attrMap(attrId).childRefs
lazy val anyChildRefsDepends = dependsOnRec(attrChildRefs, id)
attrIfd == id || anyChildRefsDepends

The main motivation was to comply with the Sonar rule.

Subjectively, I understand why this is usually recommended approach. First, it's shorter, but also it is more plural for the reader. There is less nesting and branching involved for the reader. You have entire resulted condition written in one sentence that IMO looks more natural, e.g. condition = this or that and not something_else. But I agree that a lot depends on naming. So I wouldn't hesitate choosing better names for functions, or introducing intermediate variables.

On a side note, I just noticed that attrIfd is probably a typo. Shouldn't it be attrId? Also dependsOnRef instead of dependsOnRec?

Copy link
Contributor

Choose a reason for hiding this comment

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

For simple cases, It may be ok, but here the second condition is more complex. The nesting and branching is still there, it just now must happen inside the reader's head instead of in the code. The reader must consider short-circuiting, negate one expression and operator precedence, and then build a truth table to make sense of it. The Sonar just doesn't consider any of that.

Shorter is not better, I think we should aim to minimize cognitive load.

Why do you mean by plural?

attrIfd yes typo
dependsOnRec no it means recursive - just private name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

plural -> fluent. Crazy typo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me refactor it a bit and I'll propose another variant.

@sonarcloud
Copy link

sonarcloud bot commented Aug 21, 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

Copy link
Contributor

@cerveada cerveada left a comment

Choose a reason for hiding this comment

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

LGTM, but dependsOn is good candidate to unit test

@wajda wajda merged commit dd4d4ef into develop Aug 21, 2023
21 checks passed
@wajda wajda deleted the wajda/fix-sonarcube-warnings branch August 21, 2023 15:56
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.

None yet

2 participants