Issues #1163, #1173 - refactor CPluginUserSettings

src/plugin/PluginUserSettings.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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
@@ skipping 70 matching lines @@
 
 // ENTRY POINT
 STDMETHODIMP CPluginUserSettings::QueryInterface(REFIID riid, void **ppvObj)
 {
   if (!ppvObj)
   {
     return E_POINTER;
   }
   if (riid == IID_IUnknown || riid == IID_IDispatch) // GUID comparison does not throw
   {
-    *ppvObj = static_cast<void *>(this);

[Oleksandr, 2015/02/12 04:46:22]

NIT: No space after void

[Eric, 2015/02/13 14:57:57]

Done.

+    *ppvObj = static_cast<void*>(this);
     return S_OK;
   }
   return E_NOINTERFACE;
 }
 
 /**
  * \par Limitation
  *   CPluginUserSettings is not allocated on the heap.
  *   It appears only as a member variable in CPluginTabBase.
  *   'AddRef' and 'Release' don't need reference counting because they don't present COM factories.
@@ skipping 29 matching lines @@
   {
     if (!name || !id)
     {
       return E_POINTER;
     }
     if (count != 1)
     {
       return E_FAIL;
     }
     auto item = methodIndex.find(*name); // unordered_map::find is not declared noexcept
-    if (item==methodIndex.end())

[Oleksandr, 2015/02/12 04:46:22]

NIT: Space before and after ==

[Eric, 2015/02/13 14:57:57]

Done.

+    if (item == methodIndex.end())
     {
       return DISP_E_UNKNOWNNAME;
     }
     *id = item->second;
   }
   catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
@@ skipping 62 matching lines @@
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_I4)
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)
         {
-          int indx = pDispparams->rgvarg[0].lVal;

[Oleksandr, 2015/02/12 04:46:22]

NIT: Might be a good time to fix the typo: "index"

[Eric, 2015/02/13 14:57:57]

OK.

Done.

+          int index = pDispparams->rgvarg[0].lVal;
 
           std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
 
-          if (indx < 0  ||  indx >= (int)languageTitleList.size())
+          if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
             return DISP_E_EXCEPTION;
 
           CString language;
 
-          int curIndx = 0;

[Oleksandr, 2015/02/12 04:46:22]

NIT: Typo here as well

[Eric, 2015/02/13 14:57:57]

Done.

I should point out that the algorithm here is insane.
GetFilterLanguageTitleList() is only used here, and it's constructing and
returning std::map rather than std::vector. It's only ever numerically indexed,
yet we're using a linear search to locate the container element. The associative
index that std::map provides is never used. Using std::vector would be less
code, more efficient in time, more efficient in space, and easier to understand.

I'm not fixing it with this change set as out of scope. But ... I did have to
vent.

-          for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)

[Oleksandr, 2015/02/12 04:46:22]

Nit: Space after 'for'

[Eric, 2015/02/13 14:57:57]

Done.

+          int loopIndex = 0;
+          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
-            if (curIndx == indx)
+            if (loopIndex == index)
             {
               language = it->first;
               break;
             }
-            curIndx++;
+            ++loopIndex;
           }
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(language);
         }
       }
       break;
     case dispatchID_GetLanguageTitleByIndex:
       {
         if (pDispparams->cArgs != 1)
         {
           return DISP_E_BADPARAMCOUNT;
         }
         if (pDispparams->rgvarg[0].vt != VT_I4)
         {
           return DISP_E_TYPEMISMATCH;
         }
         if (pVarResult)
         {
-          int indx = pDispparams->rgvarg[0].lVal;

[Oleksandr, 2015/02/12 04:46:22]

Nit: Another indx typo leftover and more below

[Eric, 2015/02/13 14:57:57]

Done.

+          int index = pDispparams->rgvarg[0].lVal;
 
           std::map<CString, CString> languageTitleList = settings->GetFilterLanguageTitleList();
 
-          if (indx < 0  ||  indx >= (int)languageTitleList.size())
+          if (index < 0  ||  index >= static_cast<int>(languageTitleList.size()))
             return DISP_E_EXCEPTION;
 
           CString languageTitle;
 
-          int curIndx = 0;
-          for(std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)

[Oleksandr, 2015/02/12 04:46:22]

Nit: Space after 'for'

[Eric, 2015/02/13 14:57:57]

Done.

+          int loopIndex = 0;
+          for (std::map<CString, CString>::const_iterator it = languageTitleList.begin(); it != languageTitleList.end(); ++it)
           {
-            if (curIndx == indx)
+            if (loopIndex == index)
             {
               languageTitle = it->second;
               break;
             }
-            curIndx++;
+            loopIndex++;
           }
 
           pVarResult->vt = VT_BSTR;
           pVarResult->bstrVal = SysAllocString(languageTitle);
         }
       }
       break;
     case dispatchID_SetLanguage:
       {
         if (pDispparams->cArgs != 1)
@@ skipping 151 matching lines @@
           pVarResult->vt = VT_BOOL;
           pVarResult->boolVal = CPluginClient::GetInstance()->GetPref(L"displayUpdatePage", false) ? VARIANT_TRUE : VARIANT_FALSE;
         }
       }
       break;
     default:
       return DISP_E_MEMBERNOTFOUND;
       break;
     }
   }
-  catch(...)
+  catch (...)
   {
     return E_FAIL;
   }
   return S_OK;
 }