commit 2e42e94bd769b5ffbaf4dff5cb307dfe4c25616f
author Martin Fick <martin.fick@linaro.org> Sun Jul 24 15:27:32 2022 -0600
committer Martin Fick <martin.fick@linaro.org> Wed Sep 07 14:51:06 2022 -0600
tree 4a20ea99413c4d773ede486952d5b1ee15cda920
parent ccf75ef9738e38bce6b64e75fa57245789495c9b

Use overloading for special cases instead of nulls

Replace NodeList.getProperties() with Node.getParentProperties(). This
avoids a null check on parent by relying on the ability to check the
parent node's type instead. This new method is now only defined on Node,
and not on NodeList where it was innappropriate since getProperties() on
a NodeList didn't reall make sense but it was need due to the improved
code flow.

Also take advantage of inheritance to avoid the parent == null check in
getChangeData().

Change-Id: I528631d7d5d8315e35ec5c176596a6db99f816fc

src/main/java/com/googlesource/gerrit/plugins/task/Properties.java

diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
index af8da40..4a04e2e 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/Properties.java
@@ -34,9 +34,15 @@
 
 /** Use to expand properties like ${_name} in the text of various definitions. */
 public class Properties {
-  public static final Properties EMPTY_PARENT = new Properties();
+  public static final Properties EMPTY =
+      new Properties() {
+        @Override
+        protected Function<String, String> getParentMapper() {
+          return n -> "";
+        }
+      };
 
-  protected final Properties parentProperties;
+  protected final TaskTree.Node node;
   protected final Task origTask;
   protected final CopyOnWrite<Task> task;
   protected Expander expander;
@@ -51,9 +57,9 @@
     expander = new Expander(n -> "");
   }
 
-  public Properties(Task origTask, Properties parentProperties) {
+  public Properties(TaskTree.Node node, Task origTask) {
+    this.node = node;
     this.origTask = origTask;
-    this.parentProperties = parentProperties;
     task = new CopyOnWrite<>(origTask, t -> origTask.config.new Task(t));
   }
 
@@ -103,19 +109,17 @@
         ImmutableSet.of(TaskConfig.KEY_TYPE));
   }
 
+  protected Function<String, String> getParentMapper() {
+    return n -> node.getParentProperties().expander.getValueForName(n);
+  }
+
   protected class Loader {
     protected final ChangeData changeData;
-    protected final Function<String, String> inheritedMapper;
     protected Change change;
     protected boolean isInheritedPropertyLoaded;
 
     public Loader(ChangeData changeData) {
       this.changeData = changeData;
-      if (parentProperties == null || parentProperties.expander == null) {
-        inheritedMapper = n -> "";
-      } else {
-        inheritedMapper = n -> parentProperties.expander.getValueForName(n);
-      }
     }
 
     public boolean isNonTaskDefinedPropertyLoaded() {
@@ -130,7 +134,7 @@
       if (value == null) {
         value = origTask.properties.get(name);
         if (value == null) {
-          value = inheritedMapper.apply(name);
+          value = getParentMapper().apply(name);
           if (!value.isEmpty()) {
             isInheritedPropertyLoaded = true;
           }

src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java

diff --git a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
index 06ce146..e4a2f10 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/task/TaskTree.java
@@ -124,11 +124,7 @@
     }
 
     public ChangeData getChangeData() {
-      return parent == null ? TaskTree.this.changeData : parent.getChangeData();
-    }
-
-    protected Properties getProperties() {
-      return Properties.EMPTY_PARENT;
+      return TaskTree.this.changeData;
     }
 
     protected boolean isTrusted() {
@@ -200,7 +196,7 @@
     public Node(NodeList parent, Task task) throws ConfigInvalidException, OrmException {
       this.parent = parent;
       taskKey = task.key();
-      properties = new Properties(task, parent.getProperties());
+      properties = new Properties(this, task);
       refreshTask();
     }
 
@@ -250,9 +246,8 @@
       }
     }
 
-    @Override
-    protected Properties getProperties() {
-      return properties;
+    protected Properties getParentProperties() {
+      return (parent instanceof Node) ? ((Node) parent).properties : Properties.EMPTY;
     }
 
     @Override
@@ -260,6 +255,11 @@
       return parent.isTrusted() && !task.isMasqueraded;
     }
 
+    @Override
+    public ChangeData getChangeData() {
+      return parent.getChangeData();
+    }
+
     public boolean isChange() {
       return false;
     }
@@ -320,7 +320,7 @@
           if (tasksFactory != null) {
             NamesFactory namesFactory = task.config.getNamesFactory(tasksFactory.namesFactory);
             if (namesFactory != null && namesFactory.type != null) {
-              namesFactory = getProperties().getNamesFactory(namesFactory);
+              namesFactory = properties.getNamesFactory(namesFactory);
               switch (NamesFactoryType.getNamesFactoryType(namesFactory.type)) {
                 case STATIC:
                   addStaticTypeTasks(tasksFactory, namesFactory);