Cleanup

src/plugin/AdblockPlusClient.cpp

Index: src/plugin/AdblockPlusClient.cpp
===================================================================
--- a/src/plugin/AdblockPlusClient.cpp
+++ b/src/plugin/AdblockPlusClient.cpp
@@ -9,7 +9,6 @@
 
 #include "AdblockPlusClient.h"
 
-#include "../shared/Communication.h"
 #include "../shared/Utils.h"
 
 namespace
@@ -355,11 +354,9 @@
   }
 }
 
-void CAdblockPlusClient::AddFilter(const std::wstring& text)
+// A helper method to reuse the code
+void CAdblockPlusClient::PostRequest(Communication::OutputBuffer request)

[Wladimir Palant, 2013/07/22 06:43:11]

I can't say I like the PostRequest() and FetchResponse() helpers:

1) Its purpose is non-obvious from the name. It is really
CallAdblockPlusEngineProcedure() but merely with exceptions swallowed.

2) Even if the purpose were clear, IMHO this makes the code harder to follow
because the exception handling is hidden. Instead of catching exceptions the
calling functions need to check whether they got a valid result.

So I would pretty much suggest removing these helpers. Feel free to ask Felix
for a third opinion if you disagree.

Note that you can reduce code size regardless if you use templates for the
GetPref/SetPref methods.

[Oleksandr, 2013/07/22 09:53:31]

I do disagree, at least partially. The code looks cleaner and more readable this
way IMHO. At least the PostRequest stuff seems to be worth it. Removed the
FetchResponse. Felix?
On 2013/07/22 06:43:11, Wladimir Palant wrote:

[Felix Dahlke, 2013/07/23 10:26:12]

I can see how you'd like to avoid the duplication Oleksandr, but I'm not a fan
of this either.

It looks like we're just swallowing all the exceptions anyway, so what we could
do is change CallAdblockPlusEngine so that it works like FetchResponse does now:
Return some special value on error instead of throwing an exception. What do you
think, Wladimir?

[Wladimir Palant, 2013/07/23 12:18:27]

If you look closely, we aren't really swallowing exceptions - depending on the
caller a different default behavior is in order. So FetchResponse() merely
replaces real exceptions with sort of exceptions based on return value: the
caller needs to check the return value and then go to the "default" execution
branch. There is a readability loss but IMHO no advantage whatsoever (worse: if
we forget to check the return value we might not even crash but produce wrong
results and cause issues that will be hard to investigate).

Arguably, PostRequest() has some value for the cases where we aren't interested
in the response. But then the function name needs to make the intention more
clear: CallEngineIgnoreResponse() if you like (and I would really shorten the
name of CallAdblockPlusEngineProcedure() into CallEngine()).

[Felix Dahlke, 2013/07/23 12:40:15]

What I meant was that we ignore the actual exception, we just log it in any
case. So I think we should log it in CallEngine(), not in the caller.

Then we have two options for handling errors: Use exceptions or return codes.
With exceptions it would look like this:

When we care:

try
{
  response = CallEngine(request);
}
catch (const std::exception&)
{
  return false;
}

When we don't care:

try
{
  CallEngine(request);
}
catch (const std::exception&) {}

With return codes, it could look like this:

When we care:

if (!CallEngine(request, response))
  return false;

When we don't care:

CallEngine(request);

I generally like exceptions, obviously, but I think in this case it's better to
go with the return code. We don't care what the exception is, and if my
suggestion is implemented, it's already being handled in CallEngine.

As you can see, I'm all for renaming the method to CallEngine() :)

 {
-  Communication::OutputBuffer request;
-  request << Communication::PROC_ADD_FILTER << ToUtf8String(text);
-
   try
   {
     CallAdblockPlusEngineProcedure(request);
@@ -370,37 +367,50 @@
   }
 }
 
+void CAdblockPlusClient::AddFilter(const std::wstring& text)
+{
+  Communication::OutputBuffer request;
+  request << Communication::PROC_ADD_FILTER << ToUtf8String(text);
+  PostRequest(request);
+}
+
 void CAdblockPlusClient::RemoveFilter(const std::wstring& text)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_REMOVE_FILTER << ToUtf8String(text);
-
-  try
-  {
-    CallAdblockPlusEngineProcedure(request);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
+  PostRequest(request);
 }
 
 void CAdblockPlusClient::SetPref(const std::wstring& name, const std::wstring& value)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_SET_PREF << ToUtf8String(name) << ToUtf8String(value);
-
-  try
-  {
-    CallAdblockPlusEngineProcedure(request);
-  }
-  catch (const std::exception& e)
-  {
-    DEBUG_GENERAL(e.what());
-  }
+  PostRequest(request);
 }
 
-std::wstring CAdblockPlusClient::GetPref(const std::wstring& name)
+void CAdblockPlusClient::SetPref(const std::string& name, const std::string& value)

[Wladimir Palant, 2013/07/22 06:43:11]

I don't think we need that function - the plugin always works with UTF-16, never
with UTF-8.

+{
+  Communication::OutputBuffer request;
+  request << Communication::PROC_SET_PREF << name << value;
+  PostRequest(request);
+}
+
+
+void CAdblockPlusClient::SetPref(const std::wstring& name, const int64_t & value)
+{
+  Communication::OutputBuffer request;
+  request << Communication::PROC_SET_PREF << ToUtf8String(name) << value;
+  PostRequest(request);
+}
+
+void CAdblockPlusClient::SetPref(const std::wstring& name, bool value)
+{
+  Communication::OutputBuffer request;
+  request << Communication::PROC_SET_PREF << ToUtf8String(name) << value;
+  PostRequest(request);
+}
+
+Communication::InputBuffer CAdblockPlusClient::FetchResponse(const std::wstring& name)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_PREF << ToUtf8String(name);
@@ -408,12 +418,64 @@
   try
   {
     Communication::InputBuffer response = CallAdblockPlusEngineProcedure(request);
-    std::vector<std::wstring> retValue = ReadStrings(response);
-    return retValue.size() > 0 ? retValue.at(0) : L"";
+    return response;
   }
   catch (const std::exception& e)
   {
     DEBUG_GENERAL(e.what());
-    return L"";
   }
+  return Communication::InputBuffer("");
 }
+std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, LPCWSTR defaultValue)

[Wladimir Palant, 2013/07/22 06:43:11]

I guess you have that here because const char* is implicitly converted to bool
instead of const std::wstring&? Then please actually use const char* as type
instead of LPCWSTR here, I don't see why we should use the Windows-specific
type.

+{
+  return GetPref(name, std::wstring(defaultValue));
+}
+std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const std::wstring& defaultValue)
+{
+  Communication::InputBuffer response = FetchResponse(name);
+  if (response.IsEmpty())
+    return defaultValue;
+  bool success;
+  response >> success;
+  if (success)
+  {
+    std::string value;
+    response >> value;
+    return ToUtf16String(value);
+  }
+  else
+    return defaultValue;
+}
+
+bool CAdblockPlusClient::GetPref(const std::wstring& name, bool defaultValue)
+{
+  Communication::InputBuffer response = FetchResponse(name);
+  if (response.IsEmpty())
+    return defaultValue;
+  bool success;
+  response >> success;
+  if (success)
+  {
+    bool value;
+    response >> value;
+    return value;
+  }
+  else
+    return defaultValue;
+}
+int64_t CAdblockPlusClient::GetPref(const std::wstring& name, int64_t defaultValue)
+{
+  Communication::InputBuffer response = FetchResponse(name);
+  if (response.IsEmpty())
+    return defaultValue;
+  bool success;
+  response >> success;
+  if (success)
+  {
+    int64_t value;
+    response >> value;
+    return value;
+  }
+  else
+    return defaultValue;
+}