ropensci-archive / nbaR

:warning: ARCHIVED :warning: R client library for the Netherlands Biodiversity Api (NBA)
Other
3 stars 2 forks source link

duplicated code #33

Closed hettling closed 5 years ago

hettling commented 5 years ago

The code has a lot of repetition, and is not particularly human readable presumably because it is generated automatically from the API. I'm not sure how to evaluate this part of the package: on the one hand, it is awesome that the code is programmatically generated to deal with future changes in the API, but on the other hand, there are repetitive blocks of code like the following, which itself is repeated twice in the definition of the Specimen class:

self[["sourceSystemId"]] <-
  SpecimenList[["sourceSystemId"]]
self[["recordURI"]] <-
  SpecimenList[["recordURI"]]
self[["id"]] <-
  SpecimenList[["id"]]
self[["unitID"]] <-
  SpecimenList[["unitID"]]
self[["unitGUID"]] <-
  SpecimenList[["unitGUID"]]
self[["collectorsFieldNumber"]] <-
  SpecimenList[["collectorsFieldNumber"]]
self[["assemblageID"]] <-
  SpecimenList[["assemblageID"]]
self[["sourceInstitutionID"]] <-
  SpecimenList[["sourceInstitutionID"]]
self[["sourceID"]] <-
  SpecimenList[["sourceID"]]
self[["previousSourceID"]] <-
  SpecimenList[["previousSourceID"]]
self[["owner"]] <-
  SpecimenList[["owner"]]
self[["licenseType"]] <-
  SpecimenList[["licenseType"]]
self[["license"]] <-
  SpecimenList[["license"]]
self[["recordBasis"]] <-
  SpecimenList[["recordBasis"]]
self[["kindOfUnit"]] <-
  SpecimenList[["kindOfUnit"]]
self[["collectionType"]] <-
  SpecimenList[["collectionType"]]
self[["sex"]] <-
  SpecimenList[["sex"]]
self[["phaseOrStage"]] <-
  SpecimenList[["phaseOrStage"]]
self[["title"]] <-
  SpecimenList[["title"]]
self[["notes"]] <-
  SpecimenList[["notes"]]
self[["preparationType"]] <-
  SpecimenList[["preparationType"]]
self[["previousUnitsText"]] <-
  SpecimenList[["previousUnitsText"]]
self[["numberOfSpecimen"]] <-
  SpecimenList[["numberOfSpecimen"]]
self[["fromCaptivity"]] <-
  SpecimenList[["fromCaptivity"]]
self[["objectPublic"]] <-
  SpecimenList[["objectPublic"]]
self[["multiMediaPublic"]] <-
  SpecimenList[["multiMediaPublic"]]

One reason to prefer DRY code is to avoid making the same change over and over, but if a machine is tasked with making those changes, maybe DRY matters less? (As an aside: I was somewhat unsure in reviewing whether my suggestions relate more to swagger or nbaR!) The structure of the code might make it difficult for users to glean much information by reading the source code, relative to some other packages. From my perspective this could be fine, as long as there is good documentation (for humans) that gives users the information they need.

hettling commented 5 years ago

The above code example does now only occur once in each model class, in the fromList method. The fromJSONString and toJSONString methods now use fromList and toList, respectively.