search

imsweb / data-generator

This Java library allows to create synthetic (fabricated) NAACCR data files.
Other
6 stars 1 forks source link

Clean up Sonarcloud findings #43

Closed ctmay4 closed 1 year ago

ctmay4 commented 1 year ago

In #42 the build process was updated to build in Github Actions and to automatically run Sonarcloud. The README has a link to findings.

https://sonarcloud.io/summary/overall?id=imsweb_data-generator

There are some things that should be cleaned up:

  1. Fix the 3 "bugs". They all deal with not properly closing a stream.
  2. Clean up the 29 code smells.
  3. Review the code duplication. For example, FacilityDto and FacilityFrequencyDto appear to be exactly the same. There are other as well.
ctmay4 commented 1 year ago

@caij-ims no rush on this. Whenever you are light on work.

Adding @depryf so he can follow along.

depryf commented 1 year ago

@caij-ims I merged the other issue (the one that added random text). I will wait for this one to be completed before releasing the library...

caij-ims commented 1 year ago

NaaccrDataGeneratorNaaccrDataRule is an unused abstract class with no inheritors. Can we delete it?

depryf commented 1 year ago

Yes.

caij-ims commented 1 year ago

Many of the fields in the DTOs are unused. Do we still want to keep them just in case?

depryf commented 1 year ago

It's tricky because some might be used by external programs. It's hard to tell. Can you give me a few examples?

caij-ims commented 1 year ago

PhysicianFrequencyDto Used: npi, lastName, firstName, addressState Unused: middleName, namePrefix, nameSuffix, credentials, addressFirstLine, addressSecondLine, addressCity, addressPostalCode, addressTelephone, specialty01-03

caij-ims commented 1 year ago

FacilityFrequencyDto is similar to Physician.

CityFrequencyDto doesn't use longitude/latitude.

depryf commented 1 year ago

Adding @ctmay4

I am pretty sure those three generators were added to be used in SEER*DMS but we never used them.

For now, go ahead and remove any fields that is not set by a rule.

I will submit an issue in SEER*DMS to either use those or remove them from this library...

depryf commented 1 year ago

@caij-ims I just merged a pull request. Can this issue be closed?

caij-ims commented 1 year ago

Yes