Proxy configurators

src/org/adblockplus/android/BridgeCommand.java

Index: src/org/adblockplus/android/BridgeCommand.java
diff --git a/src/org/adblockplus/android/AndroidFilterChangeCallback.java b/src/org/adblockplus/android/BridgeCommand.java
similarity index 51%
copy from src/org/adblockplus/android/AndroidFilterChangeCallback.java
copy to src/org/adblockplus/android/BridgeCommand.java
index f07836c5b4162160706fc92ea7c96df54e8235ab..e4fbb603f40a1506b8569b4818c352af72628c6d 100644
--- a/src/org/adblockplus/android/AndroidFilterChangeCallback.java
+++ b/src/org/adblockplus/android/BridgeCommand.java
@@ -17,29 +17,35 @@
 
 package org.adblockplus.android;
 
-import org.adblockplus.libadblockplus.FilterChangeCallback;
-import org.adblockplus.libadblockplus.JsValue;
-import org.adblockplus.libadblockplus.Subscription;
+import java.util.HashMap;
+import java.util.Locale;
 
-import android.content.Context;
-
-public class AndroidFilterChangeCallback extends FilterChangeCallback
+public enum BridgeCommand
 {
-  private final Context context;
+  /** Used for e.g. no-traffic check, rebuilds/redisplays notification. */
+  STATE_CHANGED,
+  UNKNOWN;
 
-  public AndroidFilterChangeCallback(final Context context)
-  {
-    this.context = context;
-  }
+  private static final HashMap<String, BridgeCommand> MAP = new HashMap<String, BridgeCommand>();
 
-  @Override
-  public void filterChangeCallback(final String action, final JsValue jsValue)
+  static
   {
-    if (action.equals("subscription.lastDownload") || action.equals("subscription.downloadStatus"))
+    final Locale locale = Locale.getDefault();

[Felix Dahlke, 2014/08/19 09:06:00]

Shouldn't we hard code EN-US here instead? Otherwise we might end up with a
locale that behaves in an unexpected way with toLowerCase and toUpperCase of
what is most definitely an ASCII string.

[René Jeschke, 2014/08/19 10:41:33]

Done.

+    for (final BridgeCommand e : BridgeCommand.values())
     {
-      final Subscription sub = new Subscription(jsValue);
-
-      Utils.updateSubscriptionStatus(this.context, sub);
+      if (e != UNKNOWN)
+      {
+        final String name = e.toString();
+        MAP.put(name, e);
+        MAP.put(name.toLowerCase(locale), e);
+        MAP.put(name.toUpperCase(locale), e);
+      }
     }
   }
+
+  public static BridgeCommand fromString(final String str)
+  {
+    final BridgeCommand e = MAP.get(str);

[Felix Dahlke, 2014/08/19 09:06:00]

I guess I'm missing something here, but wouldn't BridgeCommand.valueOf(str)
basically do the trick?

[René Jeschke, 2014/08/19 10:41:33]

'valueOf' requires that 'str' exactly matches the enum name and throws an
exception if the enum does not contain that name. I also don't know how the
different enum values are queried (I don't think there's a hashmap behind).

Performance and readability wise this is the best (safest) solution.

[Felix Dahlke, 2014/08/19 12:37:26]

This seems overengineered. If we really do need a BridgeCommand.fromString that
never throws and that can handle case insensitivity, this would do the trick:

public static BridgeCommand fromString(final String str)
{
  try
  {
    return valueOf(str.toUpperCase(Locale.ENGLISH));
  }
  catch (Throwable t)
  {
    return UNKNOWN;
  }
}

That said, I do think the best place for this logic would be the caller, i.e.
ProxyService.

[René Jeschke, 2014/08/19 12:57:36]

I just don't know why you insist on replacing a fully automatic configured, O(1)
access without any memory allocations with:

1. Stack frame and monitor/sync setup for exception catching
2. Memory allocation for 'toUpperCase'
3. Possible O(n) access for iterating over the enum
4. And: it's 8 LOC compared to two now

And con 'logic at caller':
Not everything should be solved by throwing exceptions, exceptions are not flow
control, exceptions suck and are costly in all respects.
There is a 'UNKNOWN' value defined, which makes it a default/fallback value, so
why should I throw an exception?

But, if you really think that I should replace this, I will ... though it
definitely feels wrong^^

[Felix Dahlke, 2014/08/19 13:14:57]

I just get the very strong expression that this is trying to solve a problem we
don't have - not a good thing to complicate our code base with IMHO.

How I see it, this code attempts to solve two issues:

1. The caller will pass in strings that aren't going to match the case of the
enum value.
2. Getting enum values from strings is a performance bottleneck.
3. We need to fall back to UNKNOWN if there is no enum value for the supplied
string.

Here's my take on that:

1. This is not an actual problem, we have full control over the strings we pass
in. We can make all kinds of mistakes - I don't see why we should tackle case
sensitivity specifically.
2. I really doubt it's an actual bottleneck, the lookup should pale in
comparison to the broadcast overhead. If it is, we should definitely have a
comment describing this. (And I think it'd be more logical to have the
performance hack where the actual performance issue is, i.e. in the caller)
3. This can actually happen. I'd still prefer to handle it in the caller, mainly
because it seems unintuitive to have two ways of getting an enum value from a
string with different semantics. But I wouldn't insist on it.

If you don't agree, we should get a third opinion.

[René Jeschke, 2014/08/19 13:23:53]

Well, k. But I won't leave exception handling for the caller.

+    return e != null ? e : UNKNOWN;
+  }
 }