Proxy configurators

src/org/adblockplus/android/BridgeCommand.java

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 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 enum BridgeCommand
+{
+  /** Used for e.g. no-traffic check, rebuilds/redisplays notification. */
+  STATE_CHANGED,
+  UNKNOWN;
 
-public class AndroidFilterChangeCallback extends FilterChangeCallback
-{
-  private final Context context;
+  private static final HashMap<String, BridgeCommand> MAP = new HashMap<String, BridgeCommand>();
 
-  public AndroidFilterChangeCallback(final Context context)
+  static
   {
-    this.context = context;
+    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())
+    {
+      if (e != UNKNOWN)
+      {
+        final String name = e.toString();
+        MAP.put(name, e);
+        MAP.put(name.toLowerCase(locale), e);
+        MAP.put(name.toUpperCase(locale), e);
+      }
+    }
   }
 
-  @Override
-  public void filterChangeCallback(final String action, final JsValue jsValue)
+  public static BridgeCommand fromString(final String str)
   {
-    if (action.equals("subscription.lastDownload") || action.equals("subscription.downloadStatus"))
-    {
-      final Subscription sub = new Subscription(jsValue);
-
-      Utils.updateSubscriptionStatus(this.context, sub);
-    }
+    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;
   }
 }