Add clickhouse custom data endpoints #10963
Conversation
gblaih
force-pushed
the
demo-rfc80-poc-custom-data-counts
branch
from
33790dc
to
ef2ef4f
Compare
| @Autowired | ||
| private StudyViewFilterUtil studyViewFilterUtil; | ||
| @Autowired | ||
| private CustomDataFilterUtil customDataFilterUtil; |
There was a problem hiding this comment.
Bad practice to mix constructor injection and property injection
| import org.cbioportal.web.parameter.MutationOption; | ||
| import org.cbioportal.web.parameter.Projection; | ||
| import org.cbioportal.web.parameter.StudyViewFilter; | ||
| import org.cbioportal.web.parameter.*; |
There was a problem hiding this comment.
Update your settings in intellij to not replace with a wild card
| @Autowired | ||
| private StudyViewFilterUtil studyViewFilterUtil; | ||
| @Autowired | ||
| private CustomDataFilterUtil customDataFilterUtil; |
There was a problem hiding this comment.
Update to just constructor injection
| @@ -53,12 +40,12 @@ public StudyViewFilterUtil() { | |||
| geneService = null; | |||
| } | |||
|
|
|||
| public void extractStudyAndSampleIds( | |||
| List<SampleIdentifier> sampleIdentifiers, | |||
| public <T extends SampleIdentifier> void extractStudyAndSampleIds( | |||
There was a problem hiding this comment.
In practice this is not an easily readable method... Extract study and sample ids from what.
Clean code would return a new list instead of mutating the passed in list.
There was a problem hiding this comment.
This is accepted when its obvious which object is being mutated
There was a problem hiding this comment.
it's extracting from the sampleIdentifiers parameter. i think this method was done this way because they wanted to populate two lists with a single pass of sampleIdentifiers instead of two. i'm reluctant to change it if thats how it worked before and i would have to update several usages of this. let me know if you want me to still update this
src/main/java/org/cbioportal/web/columnar/util/NewClinicalDataBinUtil.java
Show resolved
Hide resolved
| public List<ClinicalDataBin> fetchCustomDataBinCounts( | ||
| DataBinMethod dataBinMethod, | ||
| ClinicalDataBinCountFilter dataBinCountFilter, | ||
| boolean shouldRemoveSelfFromFilter |
There was a problem hiding this comment.
probably be better to utilize the GenericDataBinner
There was a problem hiding this comment.
do you mean move this method to DataBinner? i wanted to keep the clinical bin and custom bin methods together as the original clinical bin util did, also because fetching clinical bin and custom bins are very similar.
There was a problem hiding this comment.
| import org.cbioportal.web.columnar.util.NewClinicalDataBinUtil; | ||
| import org.cbioportal.web.parameter.*; | ||
| import org.cbioportal.web.util.DataBinner; | ||
| import org.cbioportal.web.util.*; |
There was a problem hiding this comment.
Update to remove wildcards
|
Can you also address the sonar cloud issues |
gblaih
force-pushed
the
demo-rfc80-poc-custom-data-counts
branch
from
faa4828
to
490ae1a
Compare
|
Implements cBioPortal/rfc80-team#18