search

imsweb / mph

Java implementation of the Multiple Primary and Histology Coding Rules.
Other
4 stars 2 forks source link

Code cleanup #109

Closed ctmay4 closed 1 year ago

ctmay4 commented 1 year ago

In #108 I switched the project integration builds from Teamcity to Github Actions and also switched to Sonarcloud (with pull requests scans integrated). There are a few things that need to be cleaned up with the code.

Spotbugs was set to not fail the build and there are some failure. Please switch the build.gradle to fail when the are Spotbugs violations. Here are the failures that need to be fixed.

H V MS: com.imsweb.mph.MphConstants.OTHER_THYROID_HISTOLOGIES is a mutable collection  At MphConstants.java:[line 165]
H V MS: com.imsweb.mph.MphConstants.ADENOCARCINOMA_PROSTATE_SUBTYPES is a mutable collection  At MphConstants.java:[line 180]
H V MS: com.imsweb.mph.MphConstants.FOLLICULAR is a mutable collection  At MphConstants.java:[line 192]
H V MS: com.imsweb.mph.MphConstants.PAPILLARY is a mutable collection  At MphConstants.java:[line 193]
H V MS: com.imsweb.mph.MphConstants.BREAST_2018_TABLE2 is a mutable collection  At MphConstants.java:[line 393]
H V MS: com.imsweb.mph.MphConstants.LUNG_2018_TABLE2 is a mutable collection  At MphConstants.java:[line 708]
H V MS: com.imsweb.mph.MphConstants.CUTANEOUS_MELANOMA_2021_TABLE2_ROWS is a mutable collection  At MphConstants.java:[line 1389]
M V MS: com.imsweb.mph.MphConstants.TESTIS_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1394]
M V MS: com.imsweb.mph.MphConstants.ESOPHAGUS_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1395]
M V MS: com.imsweb.mph.MphConstants.STOMACH_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1396]
M V MS: com.imsweb.mph.MphConstants.INTESTINE_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1397]
M V MS: com.imsweb.mph.MphConstants.ANUS_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1398]
M V MS: com.imsweb.mph.MphConstants.LIVER_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1399]
M V MS: com.imsweb.mph.MphConstants.BLADDER_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1400]
M V MS: com.imsweb.mph.MphConstants.PANCREAS_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1401]
M V MS: com.imsweb.mph.MphConstants.UTERINE_CORPUS_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1402]
M V MS: com.imsweb.mph.MphConstants.UTERINE_CERVIX_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1403]
M V MS: com.imsweb.mph.MphConstants.VULVA_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1404]
M V MS: com.imsweb.mph.MphConstants.SOFT_TISSUES_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1405]
M V MS: com.imsweb.mph.MphConstants.BONE_SITES is a mutable collection which should be package protected  At MphConstants.java:[line 1407]
H V MS: com.imsweb.mph.MphConstants.OTHER_SITES_2023_TABLE_2 is a mutable collection  At MphConstants.java:[line 2040]

There are a bunch of Sonar failures. Part of that is because there are slight differences between the ruleset in our local Sonar instance and the cloud version. We can tweak the rules if we want, but please take a look at the failures first. If you have a rule you think should be changed, let me know.

There is a badge link to the Sonarcloud failures or you can use this URL:

https://sonarcloud.io/summary/overall?id=imsweb_mph

Assigning to @bekeles and notifying @depryf

ctmay4 commented 1 year ago

This issue was closed but there were two parts. One was Spotbugs, the other was Sonarcloud code smells. Did you guys both miss that? There are 166 code smells currently.

There are a bunch of Sonar failures. Part of that is because there are slight differences between the ruleset in our local Sonar instance and the cloud version. We can tweak the rules if we want, but please take a look at the failures first. If you have a rule you think should be changed, let me know.

There is a badge link to the Sonarcloud failures or you can use this URL:

https://sonarcloud.io/summary/overall?id=imsweb_mph

ctmay4 commented 1 year ago

Adding @depryf for last comment.

depryf commented 1 year ago

I didn't close any issue. I guess it auto-closed when I merge the pull request? The changes looked good, and so I merged it. I didn't realize it wasn't all the fixes.

depryf commented 1 year ago

@bekeles what's the status of this issue now that we have merged multiple pull requests? Do you still have some things to address? I assume so since it looks like the build is still failing...

bekeles commented 1 year ago

Yeap, I am still working on them.

depryf commented 1 year ago

Thanks. Just wanted to confirm.

depryf commented 1 year ago

Getting close! Looks like the only issue is a bit of code duplication. I know you already fixed some, hopefully you can do a bit more and go under that 4% threshold. Personally I don't think this is super important, but it would be nice to make the build pass with the standard Sonarcloud setup...

ctmay4 commented 1 year ago

I would not spend a lot of time on code duplication unless it is straightforward.

ctmay4 commented 1 year ago

Wait, are committing directory to master? That should never be done.

bekeles commented 1 year ago

No we are not.

bekeles commented 1 year ago

What I don't understand is why it is failing now? The duplication was at 20.7% when we started. Is that because it is checking the new code?

ctmay4 commented 1 year ago

Yes, the branches only check new code.

bekeles commented 1 year ago

Yeah but master is failing when we merge

ctmay4 commented 1 year ago

The failures on on recent code, not overall. You changed a bunch of lines that included lines that had duplication.

image

However master still has 55 code smells. I thought you were cleaning them up?

https://sonarcloud.io/project/issues?resolved=false&types=CODE_SMELL&id=imsweb_mph

bekeles commented 1 year ago

Yeap I am. Code smell went down from 170. Those left are low priority and most of them will be fixed in my next merge.

depryf commented 1 year ago

Darn, I was also looking at the recent change instead of overall.

Let's fix the code smells and see where we are at.

bekeles commented 1 year ago

Code smells are cleaned! Duplications are down to 12.5%. Are we good?

ctmay4 commented 1 year ago

Yup, sounds good.

bekeles commented 1 year ago

Great!

ctmay4 commented 1 year ago

I never saw a pull request. Did it already get done?

depryf commented 1 year ago

Yes, I merged a bunch of them (Sewbesew preferred to split the work into different requests).

bekeles commented 1 year ago

Yeap, I have been creating a PR and Fabian was merging them one by one.