Address Code Review - Method refactor, Class rename, Builder pattern

cBioPortal · May 6, 2024 · 0726e4c · 0726e4c
1 parent 1e8560b
commit 0726e4c
Show file tree

Hide file tree

Showing 7 changed files with 52 additions and 39 deletions.
diff --git a/src/main/java/org/cbioportal/model/SampleClinicalDataCollection.java b/src/main/java/org/cbioportal/model/SampleClinicalDataCollection.java
@@ -1,19 +1,36 @@
 package org.cbioportal.model;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-public class SampleClinicalDataCollection {
+public final class SampleClinicalDataCollection {
+
+    private final Map<String, List<ClinicalData>> byUniqueSampleKey;
+
+    private SampleClinicalDataCollection(Builder builder) {
+        this.byUniqueSampleKey = Collections.unmodifiableMap(new HashMap<>(builder.byUniqueSampleKey));
+    }
 
-    private Map<String, List<ClinicalData>> byUniqueSampleKey = new HashMap<>(); 
-
     public Map<String, List<ClinicalData>> getByUniqueSampleKey() {
         return byUniqueSampleKey;
     }
 
-    public void setByUniqueSampleKey(Map<String, List<ClinicalData>> byUniqueSampleKey) {
-        this.byUniqueSampleKey = byUniqueSampleKey;
+    public static Builder builder() {
+        return new Builder();
+    }
+
+    public static class Builder {
+        private final Map<String, List<ClinicalData>> byUniqueSampleKey = new HashMap<>();
+
+        public Builder withByUniqueSampleKey(Map<String, List<ClinicalData>> byUniqueSampleKey) {
+            this.byUniqueSampleKey.putAll(byUniqueSampleKey);
+            return this;
+        }
+
+        public SampleClinicalDataCollection build() {
+            return new SampleClinicalDataCollection(this);
+        }
     }
-
 }
diff --git a/src/main/java/org/cbioportal/service/impl/ClinicalDataServiceImpl.java b/src/main/java/org/cbioportal/service/impl/ClinicalDataServiceImpl.java
@@ -16,7 +16,7 @@
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
-import static org.cbioportal.utils.Encoding.calculateBase64;
+import static org.cbioportal.utils.Encoder.calculateBase64;
 
 @Service
 public class ClinicalDataServiceImpl implements ClinicalDataService {
@@ -233,12 +233,10 @@ public List<ClinicalData> getPatientClinicalDataDetailedToSample(List<String> st
 
     @Override
     public ImmutablePair<SampleClinicalDataCollection, Integer> fetchSampleClinicalTable(List<String> studyIds, List<String> sampleIds, Integer pageSize, Integer pageNumber, String searchTerm, String sortBy, String direction) {
-
-        SampleClinicalDataCollection sampleClinicalDataCollection = new SampleClinicalDataCollection();
         if (studyIds == null || studyIds.isEmpty() || sampleIds == null || sampleIds.isEmpty()) {
-            return new ImmutablePair<>(sampleClinicalDataCollection, 0);
+            return new ImmutablePair<>(SampleClinicalDataCollection.builder().build(), 0);
         }
-        
+
         // Request un-paginated data.
         List<Integer> allSampleInternalIds = clinicalDataRepository.getVisibleSampleInternalIdsForClinicalTable(
             studyIds, sampleIds,
@@ -248,28 +246,27 @@ public ImmutablePair<SampleClinicalDataCollection, Integer> fetchSampleClinicalT
         Integer offset = PaginationCalculator.offset(pageSize, pageNumber);
 
         if (allSampleInternalIds.isEmpty() || offset >= allSampleInternalIds.size()) {
-            return new ImmutablePair<>(sampleClinicalDataCollection, 0);
+            return new ImmutablePair<>(SampleClinicalDataCollection.builder().build(), 0);
         }
 
+        return buildSampleClinicalDataCollection(allSampleInternalIds, offset, pageSize);
+    }
+
+    private ImmutablePair<SampleClinicalDataCollection, Integer> buildSampleClinicalDataCollection(List<Integer> allSampleInternalIds, Integer offset, Integer pageSize) {
+
         // Apply pagination to the sampleId list.
         Integer toIndex = PaginationCalculator.lastIndex(offset, pageSize, allSampleInternalIds.size());
         List<Integer> visibleSampleInternalIds = allSampleInternalIds.subList(offset, toIndex);
 
-        List<ClinicalData> sampleClinicalData = clinicalDataRepository.getSampleClinicalDataBySampleInternalIds(
-            visibleSampleInternalIds
-        );
-        List<ClinicalData> patientClinicalData = clinicalDataRepository.getPatientClinicalDataBySampleInternalIds(
-            visibleSampleInternalIds
-        );
-
+        List<ClinicalData> sampleClinicalData = clinicalDataRepository.getSampleClinicalDataBySampleInternalIds(visibleSampleInternalIds);
+        List<ClinicalData> patientClinicalData = clinicalDataRepository.getPatientClinicalDataBySampleInternalIds(visibleSampleInternalIds);
+
         // Merge sample and patient level clinical data and key by unique sample-key.
-        sampleClinicalDataCollection.setByUniqueSampleKey(
+        SampleClinicalDataCollection sampleClinicalDataCollection = SampleClinicalDataCollection.builder().withByUniqueSampleKey(
             Stream.concat(sampleClinicalData.stream(), patientClinicalData.stream())
-                .collect(Collectors.groupingBy(clinicalDatum ->
-                    calculateBase64(clinicalDatum.getSampleId(), clinicalDatum.getStudyId())
-        )));
-
+                .collect(Collectors.groupingBy(clinicalDatum -> calculateBase64(clinicalDatum.getSampleId(), clinicalDatum.getStudyId())))
+        ).build();
+
         return new ImmutablePair<>(sampleClinicalDataCollection, allSampleInternalIds.size());
     }
-
 }
diff --git a/...n/java/org/cbioportal/utils/Encoding.java → ...in/java/org/cbioportal/utils/Encoder.java b/...n/java/org/cbioportal/utils/Encoding.java → ...in/java/org/cbioportal/utils/Encoder.java
@@ -2,9 +2,9 @@
 
 import java.util.Base64;
 
-public class Encoding {
+public class Encoder {
 
-    private Encoding() {}
+    private Encoder() {}
 
     private static final Base64.Encoder BASE64_ENCODER = Base64.getEncoder().withoutPadding();
     private static final Base64.Decoder BASE64_DECODER = Base64.getDecoder();

diff --git a/src/main/java/org/cbioportal/web/interceptor/UniqueKeyInterceptor.java b/src/main/java/org/cbioportal/web/interceptor/UniqueKeyInterceptor.java
@@ -23,7 +23,7 @@
 
 import java.util.List;
 
-import static org.cbioportal.utils.Encoding.calculateBase64;
+import static org.cbioportal.utils.Encoder.calculateBase64;
 
 @ControllerAdvice("org.cbioportal.web")
 public class UniqueKeyInterceptor extends AbstractMappingJacksonResponseBodyAdvice {

diff --git a/src/main/java/org/cbioportal/web/util/UniqueKeyExtractor.java b/src/main/java/org/cbioportal/web/util/UniqueKeyExtractor.java
@@ -1,6 +1,6 @@
 package org.cbioportal.web.util;
 
-import org.cbioportal.utils.Encoding;
+import org.cbioportal.utils.Encoder;
 import java.util.List;
 import java.util.Collection;
 
@@ -14,8 +14,8 @@ public static void extractUniqueKeys(List<String> uniqueKeys, Collection<String>
 
     public static void extractUniqueKeys(List<String> uniqueKeys, Collection<String> studyIdsToReturn, Collection<String> patientOrSampleIdsToReturn) {
         for (String uniqueKey : uniqueKeys) {
-            String uniqueId = Encoding.decodeBase64(uniqueKey);
-            String[] patientOrSampleAndStudyId = uniqueId.split(Encoding.DELIMITER);
+            String uniqueId = Encoder.decodeBase64(uniqueKey);
+            String[] patientOrSampleAndStudyId = uniqueId.split(Encoder.DELIMITER);
             if (patientOrSampleAndStudyId.length == 2) {
                 if (patientOrSampleIdsToReturn != null) {
                     patientOrSampleIdsToReturn.add(patientOrSampleAndStudyId[0]);

diff --git a/src/test/java/org/cbioportal/service/impl/ClinicalDataServiceImplTest.java b/src/test/java/org/cbioportal/service/impl/ClinicalDataServiceImplTest.java
@@ -7,7 +7,7 @@
 import org.cbioportal.service.*;
 import org.cbioportal.service.exception.*;
 import org.cbioportal.service.util.ClinicalAttributeUtil;
-import org.cbioportal.utils.Encoding;
+import org.cbioportal.utils.Encoder;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
@@ -60,11 +60,11 @@ public void init() {
 
         datum1.setSampleId("SampleA");
         datum1.setStudyId("Study1");
-        uniqueKeySample1 = Encoding.calculateBase64("SampleA", "Study1");
+        uniqueKeySample1 = Encoder.calculateBase64("SampleA", "Study1");
 
         datum2.setSampleId("SampleA");
         datum2.setStudyId("Study2");
-        uniqueKeySample2 = Encoding.calculateBase64("SampleA", "Study2");
+        uniqueKeySample2 = Encoder.calculateBase64("SampleA", "Study2");
 
     }
 

diff --git a/src/test/java/org/cbioportal/web/StudyViewControllerTest.java b/src/test/java/org/cbioportal/web/StudyViewControllerTest.java
@@ -37,8 +37,7 @@
 import org.cbioportal.service.ViolinPlotService;
 import org.cbioportal.service.util.ClinicalAttributeUtil;
 import org.cbioportal.service.util.MolecularProfileUtil;
-import org.cbioportal.utils.Encoding;
-import org.cbioportal.web.config.CustomObjectMapper;
+import org.cbioportal.utils.Encoder;
 import org.cbioportal.web.config.TestConfig;
 import org.cbioportal.web.parameter.ClinicalDataBinCountFilter;
 import org.cbioportal.web.parameter.ClinicalDataBinFilter;
@@ -132,7 +131,7 @@ public class StudyViewControllerTest {
 
     private List<SampleIdentifier> filteredSampleIdentifiers = new ArrayList<>();
     private List<ClinicalData> clinicalData = new ArrayList<>();
-    private SampleClinicalDataCollection tableClinicalData = new SampleClinicalDataCollection();
+    private SampleClinicalDataCollection tableClinicalData;
 
     private ObjectMapper objectMapper = new ObjectMapper();
 
@@ -217,7 +216,7 @@ public void setUp() throws Exception {
         filteredSamples.add(sample1);
         filteredSamples.add(sample2);
 
-        uniqueKeySample1 = Encoding.calculateBase64(TEST_SAMPLE_ID_1, TEST_STUDY_ID);
+        uniqueKeySample1 = Encoder.calculateBase64(TEST_SAMPLE_ID_1, TEST_STUDY_ID);
 
         ClinicalData clinicalData1 = new ClinicalData();
         clinicalData1.setAttrId(TEST_ATTRIBUTE_ID);
@@ -245,7 +244,7 @@ public void setUp() throws Exception {
 
         Map<String, List<ClinicalData>> tableClinicalDataMap = new HashMap<>();
         tableClinicalDataMap.put(uniqueKeySample1, List.of(clinicalData1, clinicalData2, clinicalData3));
-        tableClinicalData.setByUniqueSampleKey(tableClinicalDataMap);
+        tableClinicalData = SampleClinicalDataCollection.builder().withByUniqueSampleKey(tableClinicalDataMap).build();
 
         reset(studyViewFilterApplier);
         reset(clinicalDataService);