Simplify RuleEngine API

src/main/java/com/team766/framework3/Rule.java

@@ -24,8 +24,8 @@
  *   public class MyRules extends RuleEngine {
  *     public MyRules() {
  *       // add rule to spin up the shooter when the boxop presses the right trigger on the gamepad
- *       rules.add(Rule.create("spin up shooter", gamepad.getButton(InputConstants.XBOX_RT)).
- *         withNewlyTriggeringProcedure(() -> new ShooterSpin(shooter)));
+ *       rules.add("spin up shooter", gamepad.getButton(InputConstants.XBOX_RT),
+ *                 () -> new ShooterSpin(shooter)));
  *       ...
  *     }
  * }
@@ -49,55 +49,6 @@ enum TriggerType {
         FINISHED
     }
 
-    /**
-     * Simple Builder for {@link Rule}s.  Configure Rules via this Builder; these fields will be immutable
-     * in the rule the Builder constructs.
-     *
-     * Instances of this Builder are created via {@link Rule#create} to simplify syntax.
-     */
-    public static class Builder {
-        private final String name;
-        private final BooleanSupplier predicate;
-        private Supplier<Procedure> newlyTriggeringProcedure;
-        private Supplier<Procedure> finishedTriggeringProcedure;
-
-        private Builder(String name, BooleanSupplier predicate) {
-            this.name = name;
-            this.predicate = predicate;
-        }
-
-        /** Specify a creator for the Procedure that should be run when this rule starts triggering. */
-        public Builder withNewlyTriggeringProcedure(Supplier<Procedure> action) {
-            this.newlyTriggeringProcedure = action;
-            return this;
-        }
-
-        public Builder withNewlyTriggeringProcedure(
-                Set<Mechanism<?>> reservations, Runnable action) {
-            this.newlyTriggeringProcedure =
-                    () -> new FunctionalInstantProcedure(reservations, action);
-            return this;
-        }
-
-        /** Specify a creator for the Procedure that should be run when this rule was triggering before and is no longer triggering. */
-        public Builder withFinishedTriggeringProcedure(Supplier<Procedure> action) {
-            this.finishedTriggeringProcedure = action;
-            return this;
-        }
-
-        public Builder withFinishedTriggeringProcedure(
-                Set<Mechanism<?>> reservations, Runnable action) {
-            this.finishedTriggeringProcedure =
-                    () -> new FunctionalInstantProcedure(reservations, action);
-            return this;
-        }
-
-        // called by {@link RuleEngine#addRule}.
-        /* package */ Rule build() {
-            return new Rule(name, predicate, newlyTriggeringProcedure, finishedTriggeringProcedure);
-        }
-    }
-
     private final String name;
     private final BooleanSupplier predicate;
     private final Map<TriggerType, Supplier<Procedure>> triggerProcedures =
@@ -106,16 +57,10 @@ public Builder withFinishedTriggeringProcedure(
             Maps.newEnumMap(TriggerType.class);
 
     private TriggerType currentTriggerType = TriggerType.NONE;
+    private boolean sealed = false;
 
-    public static Builder create(String name, BooleanSupplier predicate) {
-        return new Builder(name, predicate);
-    }
-
-    private Rule(
-            String name,
-            BooleanSupplier predicate,
-            Supplier<Procedure> newlyTriggeringProcedure,
-            Supplier<Procedure> finishedTriggeringProcedure) {
+    /* package */ Rule(
+            String name, BooleanSupplier predicate, Supplier<Procedure> newlyTriggeringProcedure) {
         if (predicate == null) {
             throw new IllegalArgumentException("Rule predicate has not been set.");
         }
@@ -131,12 +76,23 @@ private Rule(
             triggerReservations.put(
                     TriggerType.NEWLY, getReservationsForProcedure(newlyTriggeringProcedure));
         }
+    }
 
-        if (finishedTriggeringProcedure != null) {
-            triggerProcedures.put(TriggerType.FINISHED, finishedTriggeringProcedure);
-            triggerReservations.put(
-                    TriggerType.FINISHED, getReservationsForProcedure(finishedTriggeringProcedure));
+    /** Specify a creator for the Procedure that should be run when this rule was triggering before and is no longer triggering. */
+    public Rule withFinishedTriggeringProcedure(Supplier<Procedure> action) {
+        if (sealed) {
+            throw new IllegalStateException(
+                    "Cannot modify rules once they've been evaluated in the RuleEngine");
         }
+
+        triggerProcedures.put(TriggerType.FINISHED, action);
+        triggerReservations.put(TriggerType.FINISHED, getReservationsForProcedure(action));
+        return this;
+    }
+
+    public Rule withFinishedTriggeringProcedure(Set<Mechanism<?>> reservations, Runnable action) {
+        return withFinishedTriggeringProcedure(
+                () -> new FunctionalInstantProcedure(reservations, action));
     }
 
     private Set<Mechanism<?>> getReservationsForProcedure(Supplier<Procedure> supplier) {
@@ -157,6 +113,10 @@ public String getName() {
         return currentTriggerType;
     }
 
+    /* package */ void seal() {
+        sealed = true;
+    }
+
     /* package */ void reset() {
         currentTriggerType = TriggerType.NONE;
     }

src/main/java/com/team766/framework3/RuleEngine.java

@@ -10,10 +10,11 @@
 import edu.wpi.first.wpilibj2.command.CommandScheduler;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.function.BooleanSupplier;
+import java.util.function.Supplier;
 
 /**
  * {@link RuleEngine}s manage and process a set of {@link Rule}s.  Subclasses should add rules via
@@ -30,9 +31,10 @@ public class RuleEngine implements LoggingBase {
 
     private static record RuleAction(Rule rule, Rule.TriggerType triggerType) {}
 
-    private final List<Rule> rules = new LinkedList<>();
+    private final LinkedHashMap<String, Rule> rules = new LinkedHashMap<>();
     private final Map<Rule, Integer> rulePriorities = new HashMap<>();
     private BiMap<Command, RuleAction> ruleMap = HashBiMap.create();
+    private boolean sealed = false;
 
     protected RuleEngine() {}
 
@@ -41,20 +43,28 @@ public Category getLoggerCategory() {
         return Category.RULES;
     }
 
-    protected void addRule(Rule.Builder builder) {
-        Rule rule = builder.build();
-        rules.add(rule);
+    protected Rule addRule(String name, BooleanSupplier condition, Supplier<Procedure> action) {
+        Rule rule = new Rule(name, condition, action);
+        rules.put(name, rule);
         int priority = rulePriorities.size();
         rulePriorities.put(rule, priority);
+        return rule;
+    }
+
+    protected Rule addRule(
+            String name, BooleanSupplier condition, Mechanism<?> mechanism, Runnable action) {
+        return addRule(
+                name, condition, () -> new FunctionalInstantProcedure(Set.of(mechanism), action));
     }
 
     @VisibleForTesting
-    /* package */ Map<String, Rule> getRuleNameMap() {
-        Map<String, Rule> namedRules = new HashMap<>();
-        for (Rule rule : rules) {
-            namedRules.put(rule.getName(), rule);
-        }
-        return namedRules;
+    /* package */ int size() {
+        return rules.size();
+    }
+
+    @VisibleForTesting
+    /* package */ Rule getRuleByName(String name) {
+        return rules.get(name);
     }
 
     @VisibleForTesting
@@ -73,15 +83,26 @@ protected Rule getRuleForTriggeredProcedure(Command command) {
         return (ruleAction == null) ? null : ruleAction.rule;
     }
 
+    private void sealRules() {
+        for (Rule rule : rules.values()) {
+            rule.seal();
+        }
+    }
+
     public final void run() {
+        if (!sealed) {
+            sealRules();
+            sealed = true;
+        }
+
         Set<Mechanism<?>> mechanismsToUse = new HashSet<>();
 
         // TODO(MF3): when creating a Procedure, check that the reservations are the same as
         // what the Rule pre-computed.
 
         // evaluate each rule
         ruleLoop:
-        for (Rule rule : rules) {
+        for (Rule rule : rules.values()) {
             try {
                 rule.evaluate();