Merge pull request #87 from juleskers/holiday-hacking-reviews

Holiday hacking reviews
nicolasbrailo · Jan 6, 2024 · 216017b · 216017b
2 parents 0957002 + d01b39a
commit 216017b
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 15 deletions.
diff --git a/app/src/main/java/com/nicobrailo/pianoli/AppConfigTrigger.java b/app/src/main/java/com/nicobrailo/pianoli/AppConfigTrigger.java
@@ -1,6 +1,7 @@
 package com.nicobrailo.pianoli;
 
 import android.util.Log;
+import androidx.annotation.NonNull;
 
 import java.util.Arrays;
 import java.util.HashSet;
@@ -44,6 +45,13 @@ class AppConfigTrigger implements PianoListener {
      * </p>
      */
     private final Set<Integer> pressedConfigKeys = new HashSet<>();
+
+    /**
+     * User frustration tracker: how badly are they failing to open the config?
+     *
+     * @see #cb
+     * @see #setConfigRequestCallback(AppConfigCallback)
+     */
     private TooltipReminder tooltipReminder;
 
     /**
@@ -52,17 +60,29 @@ class AppConfigTrigger implements PianoListener {
     private int nextExpectedKey;
 
     /**
-     * Our "upstream", who knows enougjh about global app context to actually <em>do</em> stuff.
+     * Our "upstream", who knows enough about global app context to actually <em>do</em> stuff.
+     *
+     * @see #tooltipReminder
+     * @see #setConfigRequestCallback(AppConfigCallback)
      */
     private AppConfigCallback cb = null;
 
     AppConfigTrigger() {
         nextExpectedKey = calculateNextExpectedKey();
     }
 
-    void setConfigRequestCallback(AppConfigCallback cb) {
+    /**
+     * Dependency injection for context-handling stuff: switching to settings activity, and showing toasts.
+     *
+     * <p>
+     * Since this callback is <em>required</em> for this trigger to do anything at all, it would have been preferable
+     * to require this is provided in the constructor. Alas, the way we initialise our upstream <code>PianoCanvas</code>
+     * via XML definition precludes this.
+     * </p>
+     */
+    void setConfigRequestCallback(@NonNull AppConfigCallback cb) {
         this.cb = cb;
-        tooltipReminder = new TooltipReminder(cb);
+        this.tooltipReminder = new TooltipReminder(cb);
     }
 
     /**
@@ -144,9 +164,7 @@ public void onKeyDown(int keyIdx) {
                 // Sequence complete!
                 reset(); // clear it so it's no longer counted as in-progress.
                 // Open Sesame!
-                if (cb != null) {
-                    cb.requestConfig();
-                }
+                cb.requestConfig();
             } else {
                 nextExpectedKey = calculateNextExpectedKey();
             }

diff --git a/app/src/main/java/com/nicobrailo/pianoli/Piano.java b/app/src/main/java/com/nicobrailo/pianoli/Piano.java
@@ -1,10 +1,12 @@
 package com.nicobrailo.pianoli;
 
 import android.util.Log;
+import androidx.annotation.NonNull;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Backing model / state of our virtual piano keyboard.
@@ -140,11 +142,11 @@ private boolean isOutOfRange(int key_idx) {
      * @return Per the {@link java.util.Collection#add(Object)} contract, <code>true</code> if the listener list changed as a result of this add,
      *          <code>false</code> if it was already subscribed.
      */
-    public boolean addListener(PianoListener l) {
-        if (l != null  // avoid NullPointerExceptions on notify
-                && !listeners.contains(l)) { // don't double-add listeners, to avoid double-triggers
-            listeners.add(l);
-            return true;
+    public boolean addListener(@NonNull PianoListener l) {
+        Objects.requireNonNull(l, "Listeners must not be null to avoid NullPointerExceptions on notify");
+
+        if (!listeners.contains(l)) { // don't double-add listeners, to avoid double-triggers
+            return listeners.add(l);
         }
         return false;
     }

diff --git a/app/src/main/java/com/nicobrailo/pianoli/PianoCanvas.java b/app/src/main/java/com/nicobrailo/pianoli/PianoCanvas.java
@@ -126,7 +126,12 @@ public void reInitPiano(Context context, String prefSoundset) {
         Log.i("PianOli::PianoCanvas", "re-initialising Piano - DONE");
     }
 
-    public void setConfigRequestCallback(AppConfigTrigger.AppConfigCallback cb) {
+    /**
+     * Dependency injection for context-handling stuff: switching to settings activity, and showing toasts.
+     *
+     * @see AppConfigTrigger#setConfigRequestCallback(AppConfigTrigger.AppConfigCallback)
+     */
+    public void setConfigRequestCallback(@NonNull AppConfigTrigger.AppConfigCallback cb) {
         this.appConfigTrigger.setConfigRequestCallback(cb);
     }
 

diff --git a/app/src/test/java/com/nicobrailo/pianoli/PianoListenerTest.java b/app/src/test/java/com/nicobrailo/pianoli/PianoListenerTest.java
@@ -77,12 +77,13 @@ public void doubleRemoval() {
 
     @Test
     public void nullSafeAdd() {
-        assertFalse(piano.addListener(null), "null-listener should be silently accepted, but not do anything");
+        //noinspection DataFlowIssue // intentionally violating @NonNull to test handling.
+        assertThrows(NullPointerException.class, () -> piano.addListener(null), "null-listener should not be accepted");
 
         assertDoesNotThrow(() -> piano.doKeyDown(0),
-                "after adding a null-listener, notification should not explode");
+                "after adding a null-listener, listener notification should not explode");
         assertDoesNotThrow(() -> piano.doKeyUp(0),
-                "after adding a null-listener, notification should not explode");
+                "after adding a null-listener, listener notification should not explode");
     }
 
     @Test