sc: simplify GetByName, FreeTable methods of DPCollection + test

Simplify DPCollection GetByName and FreeTable by using c++11
features. Change GetByName to return non-const ScDPObject as this
is more useful (as operator[] does that already anyway).

Change-Id: Ia502c615acc5cb7fdc51acea9b428d04e1c9a40f
Reviewed-on: https://gerrit.libreoffice.org/34002
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>

4 files changed, 109 insertions, 17 deletions

diff --git a/sc/inc/dpobject.hxx b/sc/inc/dpobject.hxx
index 2f144c5c7786..d8c20f10f5f8 100644
--- a/sc/inc/dpobject.hxx
+++ b/sc/inc/dpobject.hxx
@@ -371,7 +371,7 @@ public:
     SC_DLLPUBLIC ScDPObject& operator[](size_t nIndex);
     SC_DLLPUBLIC const ScDPObject& operator[](size_t nIndex) const;
 
-    const ScDPObject* GetByName(const OUString& rName) const;
+    ScDPObject* GetByName(const OUString& rName) const;
 
     void DeleteOnTab( SCTAB nTab );
     void UpdateReference( UpdateRefMode eUpdateRefMode,
diff --git a/sc/qa/unit/ucalc.hxx b/sc/qa/unit/ucalc.hxx
index 17154478da0d..f3247486ffa3 100644
--- a/sc/qa/unit/ucalc.hxx
+++ b/sc/qa/unit/ucalc.hxx
@@ -320,6 +320,11 @@ public:
      */
     void testPivotTableRepeatItemLabels();
 
+    /**
+     * Test DPCollection public methods
+     */
+    void testPivotTableDPCollection();
+
     void testCellCopy();
     void testSheetCopy();
     void testSheetMove();
@@ -621,6 +626,7 @@ public:
     CPPUNIT_TEST(testPivotTableFieldReference);
     CPPUNIT_TEST(testPivotTableDocFunc);
     CPPUNIT_TEST(testPivotTableRepeatItemLabels);
+    CPPUNIT_TEST(testPivotTableDPCollection);
     CPPUNIT_TEST(testCellCopy);
     CPPUNIT_TEST(testSheetCopy);
     CPPUNIT_TEST(testSheetMove);
diff --git a/sc/qa/unit/ucalc_pivottable.cxx b/sc/qa/unit/ucalc_pivottable.cxx
index a7c503748f35..28b475d029b8 100644
--- a/sc/qa/unit/ucalc_pivottable.cxx
+++ b/sc/qa/unit/ucalc_pivottable.cxx
@@ -2401,5 +2401,93 @@ void Test::testPivotTableRepeatItemLabels()
     m_pDoc->DeleteTab(0);
 }
 
+void Test::testPivotTableDPCollection()
+{
+    m_pDoc->InsertTab(0, "Data");
+    m_pDoc->InsertTab(1, "Table");
+
+    // Dimension definition
+    DPFieldDef aFields[] = {
+        { "Software", sheet::DataPilotFieldOrientation_ROW, 0, false },
+        { "Version",  sheet::DataPilotFieldOrientation_COLUMN, 0, false },
+        { "1.2.3",    sheet::DataPilotFieldOrientation_DATA, 0, false }
+    };
+
+    // Raw data
+    const char* aData[][3] = {
+        { "LibreOffice", "3.3.0", "30" },
+        { "LibreOffice", "3.3.1", "20" },
+        { "LibreOffice", "3.4.0", "45" },
+    };
+
+    size_t nFieldCount = SAL_N_ELEMENTS(aFields);
+    size_t nDataCount = SAL_N_ELEMENTS(aData);
+
+    ScRange aSrcRange = insertDPSourceData(m_pDoc, aFields, nFieldCount, aData, nDataCount);
+    SCROW nRow1 = aSrcRange.aStart.Row(), nRow2 = aSrcRange.aEnd.Row();
+    SCCOL nCol1 = aSrcRange.aStart.Col(), nCol2 = aSrcRange.aEnd.Col();
+    ScRange aDataRange(nCol1, nRow1, 0, nCol2, nRow2, 0);
+
+    ScDPCollection* pDPs = m_pDoc->GetDPCollection();
+    bool bSuccess = false;
+
+    // Check at the beginning
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("there should be no DP table", size_t(0), pDPs->GetCount());
+
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("DP1"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName(""));
+
+    // Add 2 DP objects
+    ScDPObject* pDPObj = createDPFromRange(m_pDoc, aDataRange , aFields, nFieldCount, false);
+    bSuccess = pDPs->InsertNewTable(pDPObj);
+    CPPUNIT_ASSERT_MESSAGE("failed to insert a new DP object into document", bSuccess);
+    pDPObj->SetName("DP1"); // set custom name
+
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("there should be only one data pilot table.", size_t(1), pDPs->GetCount());
+
+    ScDPObject* pDPObj2 = createDPFromRange(m_pDoc, aDataRange, aFields, nFieldCount, false);
+    bSuccess = pDPs->InsertNewTable(pDPObj2);
+    CPPUNIT_ASSERT_MESSAGE("failed to insert a new DP object into document", bSuccess);
+    pDPObj2->SetName("DP2"); // set custom name
+
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("there should be two DP tables", size_t(2), pDPs->GetCount());
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("should return first DPObject",
+                                 pDPObj, pDPs->GetByName("DP1"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("should return second DPObject",
+                                 pDPObj2, pDPs->GetByName("DP2"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("empty string should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName(""));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("non existent name should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("Non"));
+    // Remove first DP Object
+    pDPs->FreeTable(pDPObj);
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("there should be only one DP table", size_t(1), pDPs->GetCount());
+
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("first DP object was deleted, should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("DP1"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("should return second DPObject",
+                                 pDPObj2, pDPs->GetByName("DP2"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("empty string should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName(""));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("non existent name should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("Non"));
+
+    // Remove second DP Object
+    pDPs->FreeTable(pDPObj2);
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("first DP object was deleted, should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("DP1"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("second DP object was deleted, should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("DP2"));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("empty string should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName(""));
+    CPPUNIT_ASSERT_EQUAL_MESSAGE("non existent name should return nullptr",
+                                 static_cast<ScDPObject*>(nullptr), pDPs->GetByName("Non"));
+
+    // Clean-up
+    m_pDoc->DeleteTab(1);
+    m_pDoc->DeleteTab(0);
+}
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/source/core/data/dpobject.cxx b/sc/source/core/data/dpobject.cxx
index c413220b8598..9b7c6e2bdb26 100644
--- a/sc/source/core/data/dpobject.cxx
+++ b/sc/source/core/data/dpobject.cxx
@@ -3734,12 +3734,13 @@ const ScDPObject& ScDPCollection::operator [](size_t nIndex) const
     return *maTables[nIndex].get();
 }
 
-const ScDPObject* ScDPCollection::GetByName(const OUString& rName) const
+ScDPObject* ScDPCollection::GetByName(const OUString& rName) const
 {
-    TablesType::const_iterator itr = maTables.begin(), itrEnd = maTables.end();
-    for (; itr != itrEnd; ++itr)
-        if ((*itr)->GetName() == rName)
-            return itr->get();
+    for (std::unique_ptr<ScDPObject> const & pObject : maTables)
+    {
+        if (pObject->GetName() == rName)
+            return pObject.get();
+    }
 
     return nullptr;
 }
@@ -3771,22 +3772,19 @@ OUString ScDPCollection::CreateNewName() const
     return OUString();                    // should not happen
 }
 
-void ScDPCollection::FreeTable(ScDPObject* pDPObj)
+void ScDPCollection::FreeTable(ScDPObject* pDPObject)
 {
-    const ScRange& rOutRange = pDPObj->GetOutRange();
+    const ScRange& rOutRange = pDPObject->GetOutRange();
     const ScAddress& s = rOutRange.aStart;
     const ScAddress& e = rOutRange.aEnd;
     mpDoc->RemoveFlagsTab(s.Col(), s.Row(), e.Col(), e.Row(), s.Tab(), ScMF::DpTable);
-    TablesType::iterator itr = maTables.begin(), itrEnd = maTables.end();
-    for (; itr != itrEnd; ++itr)
+
+    auto funcRemoveCondition = [pDPObject] (std::unique_ptr<ScDPObject> const & pCurrent)
     {
-        ScDPObject* p = itr->get();
-        if (p == pDPObj)
-        {
-            maTables.erase(itr);
-            break;
-        }
-    }
+        return pCurrent.get() == pDPObject;
+    };
+
+    maTables.erase(std::remove_if(maTables.begin(), maTables.end(), funcRemoveCondition), maTables.end());
 }
 
 bool ScDPCollection::InsertNewTable(ScDPObject* pDPObj)