search

alfasoftware / astra

Astra: a Java tool for analysing and refactoring Java source code
Apache License 2.0
68 stars 35 forks source link

Reduce cognitive complexity of AstraUtils#isStaticallyImportedMethod #89

Closed RadikalJin closed 2 years ago

RadikalJin commented 2 years ago

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

dasilva-gabriel commented 2 years ago

Hello @RadikalJin ,

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

Thank you 👌

RadikalJin commented 2 years ago

Hello @cmuagab,

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

dasilva-gabriel commented 2 years ago

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

RadikalJin commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/alfasoftware/astra/pull/92/commits/f6b77be1c6977bd7f1b26298b6d7dc3222bd2a1a#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.