Code review feedback

Author: labkey-tchad

src/org/labkey/test/components/ui/grids/FieldReferenceManager.java

@@ -18,25 +18,25 @@
 
 public class FieldReferenceManager
 {
-    private final List<FieldReference> fieldReferences;
-    private final Map<Integer, FieldReference> fieldsByIndex;
-    private final Map<FieldKey, FieldReference> fieldKeys = new LinkedHashMap<>();
-    private final Map<String, FieldReference> fieldLabels = new LinkedHashMap<>();
+    private final List<FieldReference> _fieldReferences;
+    private final Map<Integer, FieldReference> _fieldsByIndex;
+    private final Map<FieldKey, FieldReference> _fieldKeys = new LinkedHashMap<>();
+    private final Map<String, FieldReference> _fieldLabels = new LinkedHashMap<>();
 
     public <T extends FieldReference> FieldReferenceManager(List<T> columnHeaders)
     {
-        fieldReferences = List.copyOf(columnHeaders);
-        fieldsByIndex = columnHeaders.stream().collect(Collectors.toMap(FieldReference::getDomIndex, Function.identity()));
+        _fieldReferences = List.copyOf(columnHeaders);
+        _fieldsByIndex = columnHeaders.stream().collect(Collectors.toMap(FieldReference::getDomIndex, Function.identity()));
     }
 
     public List<FieldReference> getColumnHeaders()
     {
-        return fieldReferences;
+        return _fieldReferences;
     }
 
     public FieldReference getColumnHeader(int index)
     {
-        return fieldsByIndex.get(index);
+        return _fieldsByIndex.get(index);
     }
 
     /**
@@ -86,18 +86,18 @@ public FieldReference getColumnHeader(int index)
 
     private FieldReference findColumnHeaderByFieldKey(FieldKey fieldIdentifier)
     {
-        if (fieldKeys.containsKey(fieldIdentifier))
+        if (_fieldKeys.containsKey(fieldIdentifier))
         {
-            return fieldKeys.get(fieldIdentifier);
+            return _fieldKeys.get(fieldIdentifier);
         }
-        else if (fieldKeys.size() < fieldReferences.size())
+        else if (_fieldKeys.size() < _fieldReferences.size())
         {
-            for (FieldReference header : fieldReferences)
+            for (FieldReference header : _fieldReferences)
             {
-                if (!fieldKeys.containsValue(header))
+                if (!_fieldKeys.containsValue(header))
                 {
                     FieldKey fieldKey = header.getFieldKey();
-                    fieldKeys.put(fieldKey, header);
+                    _fieldKeys.put(fieldKey, header);
                     if (fieldKey.equals(fieldIdentifier))
                     {
                         return header;
@@ -111,18 +111,18 @@ else if (fieldKeys.size() < fieldReferences.size())
 
     private FieldReference findColumnHeaderByLabel(String label)
     {
-        if (fieldLabels.containsKey(label))
+        if (_fieldLabels.containsKey(label))
         {
-            return fieldLabels.get(label);
+            return _fieldLabels.get(label);
         }
-        else if (fieldLabels.size() < fieldReferences.size())
+        else if (_fieldLabels.size() < _fieldReferences.size())
         {
-            for (FieldReference header : fieldReferences)
+            for (FieldReference header : _fieldReferences)
             {
-                if (!fieldLabels.containsValue(header))
+                if (!_fieldLabels.containsValue(header))
                 {
                     String columnLabel = header.getLabel();
-                    fieldLabels.put(columnLabel, header);
+                    _fieldLabels.put(columnLabel, header);
                     if (columnLabel.equals(label))
                     {
                         return header;

src/org/labkey/test/tests/TimeChartAPITest.java

@@ -312,13 +312,13 @@ private void testVisApi(File htmlPage, String[] testTitles, @Nullable int[] test
                 waitForElement(Locator.paginationText(testRowCounts[testIndex]), WAIT_FOR_JAVASCRIPT);
             }
 
-            CachingSupplier<DataRegionTable> table = new CachingSupplier<>(
-                () -> new DataRegionTable("apiTestDataRegion", this));
+            DataRegionTable table = new DataRegionTable.DataRegionFinder(getDriver())
+                .withName("apiTestDataRegion").findWhenNeeded();
 
             if (testColumnNames != null)
             {
                 List<String> expectedColumnNames = Arrays.asList(testColumnNames[testIndex]);
-                List<String> columnNames = new ArrayList<>(table.get().getColumnNames());
+                List<String> columnNames = new ArrayList<>(table.getColumnNames());
 
                 if (!columnNames.containsAll(expectedColumnNames))
                 {
@@ -333,14 +333,14 @@ private void testVisApi(File htmlPage, String[] testTitles, @Nullable int[] test
             {
                 Pair<String, List<Object>> expectedColumn = expectedColForAllTests.get(testIndex);
                 String columnName = expectedColumn.getKey();
-                int columnIndex = table.get().getColumnIndex(columnName);
+                int columnIndex = table.getColumnIndex(columnName);
                 List<Object> expectedValues = expectedColumn.getValue();
                 List<Object> actualValues = new ArrayList<>();
                 boolean isNumberCol = expectedValues.get(0) instanceof Number;
 
-                for (int i = 0; i < table.get().getDataRowCount() && actualValues.size() < expectedValues.size() && columnIndex >= 0; i++)
+                for (int i = 0; i < table.getDataRowCount() && actualValues.size() < expectedValues.size() && columnIndex >= 0; i++)
                 {
-                    String value = table.get().getDataAsText(i, columnIndex).trim();
+                    String value = table.getDataAsText(i, columnIndex).trim();
                     if (!value.isEmpty())
                         actualValues.add(isNumberCol ? Double.parseDouble(value) : value);
                 }

src/org/labkey/test/util/CachingSupplier.java

@@ -2,6 +2,28 @@
 
 import java.util.function.Supplier;
 
+/**
+ * Wraps another Supplier, invoking it lazily and caching its results for subsequent calls to get().<br>
+ * Intended to be used as an alternative to null-checking and populating a member variable.<br>
+ * Similar to {@link org.labkey.test.selenium.LazyWebElement} but for other types.
+ * <pre>{@code
+ * final CachingSupplier<T> item = new CachingSupplier<>(this::computeItem);
+ * T getItem() {
+ *     return item.get();
+ * }
+ * }</pre>
+ * <pre>{@code
+ * // Old pattern
+ * T item;
+ * T getItem() {
+ *     if (item == null)
+ *     {
+ *         item = computeItem();
+ *     }
+ *     return item;
+ * }
+ * }</pre>
+ */
 public class CachingSupplier<T> implements Supplier<T>
 {
     private final Supplier<T> _factory;