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

Fix Sonar S1130 throws declarations should not be superfluous #1062

Merged
merged 1 commit into from Jun 1, 2023

Conversation

jlerbsc
Copy link
Contributor

@jlerbsc jlerbsc commented May 21, 2023

This is a fix on #1061

The goal is to remove all unnecessary exception declarations or that overload the method declaration. All RuntimeExceptions are suppressed as they are automatically propagated by Java. When the method declaration contains a checked exception as well as a parent exception, only the parent exception is kept because the derived exception is also automatically picked up by the parent exception declaration.

Our automatic defect correction solution detected and corrected 72 violations of the Sonar S1130 rule '"throws" declarations should not be superfluous'. According to Sonar, it would have taken 6 hours to identify and correct these violations.

We hope that this PR suits you and allows you to improve the quality of your project.

Copy link
Contributor

@josemduarte josemduarte left a comment

Choose a reason for hiding this comment

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

Very nice indeed, thank you! LGTM

@josemduarte josemduarte merged commit 56b98c8 into biojava:master Jun 1, 2023
1 check passed
@josemduarte
Copy link
Contributor

I was trying to see if the number of code smells went down after this merge. But it seems not: https://sonarcloud.io/project/activity?id=biojava_biojava&graph=issues (I hope you can see this @jlerbsc ? let me know if not). Any ideas what's happening? Perhaps our sonar setup is not right somehow?

@jlerbsc
Copy link
Contributor Author

jlerbsc commented Jun 3, 2023 via email

@jlerbsc
Copy link
Contributor Author

jlerbsc commented Jun 13, 2023

Sorry for the late reply but I am currently on vacation.
I think that if you filter the issues of this new version of the project by the issue code "s1130" you will probably see residual issues that are not handled by our solution.
An exception in a throws declaration in Java is superfluous if it is:

  • listed multiple times
  • a subclass of another listed exception
  • completely unnecessary because the declared exception type cannot actually be thrown

The type of issue not covered actually is "completely unnecessary because the declared exception type cannot actually be thrown".

There may be cases that are not fully covered by our solution, I will look at this when I return from leave. Best wishes.

@jlerbsc
Copy link
Contributor Author

jlerbsc commented Jun 20, 2023

I found that there are indeed 5 cases that were not covered when running our solution although they matched "a subclass of another listed exception" as they appeared in constructors while our solution only parsed method declarations.

We have upgraded our solution to take constructor declarations into account. Would you like me to propose a new PR to complete the corrections on the sonar rule s1130 "throws declarations should not be superfluous"?

Below is the summary of the execution

Executed on 2023-06-20T10:38:37 on class org\biojava\nbio\ronn\ORonn.java with rule S1130 detected 2 defect(s)
Executed on 2023-06-20T10:38:37 on class org\biojava\nbio\ronn\ORonnModel.java with rule S1130 detected 1 defect(s)
Executed on 2023-06-20T10:38:45 on class org\biojava\nbio\structure\align\util\SynchronizedOutFile.java with rule S1130 detected 2 defect(s)

The corrections relate to the removal of the declaration of the exception NumberFormatException which is a RuntimeException as well as the removal of the declaration of FileNotFoundException when it is associated with IOException because FileNotFoundException is a subtype of IOException

@josemduarte
Copy link
Contributor

Thanks for explaining. Yes we'd be happy to review a new PR.

@jlerbsc
Copy link
Contributor Author

jlerbsc commented Jun 21, 2023

Here is the corresponding PR #1065

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