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

Reduce cognitive complexity of AstraUtils#isStaticallyImportedMethod #89

Closed
RadikalJin opened this issue Apr 25, 2022 · 6 comments · Fixed by #92
Closed

Reduce cognitive complexity of AstraUtils#isStaticallyImportedMethod #89

RadikalJin opened this issue Apr 25, 2022 · 6 comments · Fixed by #92
Assignees
Labels
good first issue Good for newcomers

Comments

@RadikalJin
Copy link
Member

See https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&id=alfasoftware_astra&open=AXhfYNiw01lXSQw-3UdK

@RadikalJin RadikalJin added the good first issue Good for newcomers label Apr 25, 2022
@dasilva-gabriel
Copy link
Contributor

Hello @RadikalJin ,

I want to try on this function :) Can I be assigned?

Thank you 👌

@RadikalJin
Copy link
Member Author

Hello @cmuagab,

Sure! Thanks for your interest in contributing to Astra. I'll assign this over to you now.

@dasilva-gabriel
Copy link
Contributor

If my calculations are correct I have a cognitive complexity of 11 👍

@RadikalJin
Copy link
Member Author

Thanks very much for submitting this PR, @cmuagab - it looks like breaking the function up into smaller functions is the way to go.
Unfortunately it looks like there has been a functional change - see the tests in TestMethodInvocationRefactor and TestImportsRefactor, in this build: https://github.com/alfasoftware/astra/runs/6294952071?check_suite_focus=true.

@dasilva-gabriel
Copy link
Contributor

Thanks very much for submitting this PR, @cmuagab - it looks like breaking the function up into smaller functions is the way to go. Unfortunately it looks like there has been a functional change - see the tests in TestMethodInvocationRefactor and TestImportsRefactor, in this build: https://github.com/alfasoftware/astra/runs/6294952071?check_suite_focus=true.

Thank you for your responsiveness ! 😍
I made the changes, the tests are ok.
My cognitive complexity is 13 according to me.

@RadikalJin
Copy link
Member Author

Great stuff! I've made a couple of tweaks - mostly formatting but also noticed that, although not caught by any unit test - there was a potential issue with returning false too early here: f6b77be#diff-d04ef1893e85db481c55f07e071e0e14bc32303f660d1612c23cff78a2c05767R652
I've tweaked it so that if the boolean is false, we continue checking the rest of the imports.

This is a really nice change, thank you for your contribution to Astra, @cmuagab! Merging now.

RadikalJin added a commit that referenced this issue May 5, 2022
(#89) Reduce cognitive complexity of AstraUtils#isStaticallyImportedMethod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants