Refactor function structs into lambdas (#5675)

* change signature to use lambda

* reuse comparator

* inline structs in forEachCard

* inline structs

* Refactor exportDeckToDecklist

* fix unit test

cockatrice/src/client/tapped_out_interface.cpp

@@ -92,16 +92,9 @@ void TappedOutInterface::analyzeDeck(DeckList *deck)
     manager->post(request, data);
 }
 
-struct CopyMainOrSide
+void TappedOutInterface::copyDeckSplitMainAndSide(DeckList &source, DeckList &mainboard, DeckList &sideboard)
 {
-    CardDatabase &cardDatabase;
+    auto copyMainOrSide = [this, &mainboard, &sideboard](const auto node, const auto card) {
-    DeckList &mainboard, &sideboard;
-
-    CopyMainOrSide(CardDatabase &_cardDatabase, DeckList &_mainboard, DeckList &_sideboard)
-        : cardDatabase(_cardDatabase), mainboard(_mainboard), sideboard(_sideboard){};
-
-    void operator()(const InnerDecklistNode *node, const DecklistCardNode *card) const
-    {
         CardInfoPtr dbCard = cardDatabase.getCard(card->getName());
         if (!dbCard || dbCard->getIsToken())
             return;
@@ -112,11 +105,7 @@ struct CopyMainOrSide
         else
             addedCard = mainboard.addCard(card->getName(), node->getName());
         addedCard->setNumber(card->getNumber());
-    }
+    };
-};
 
-void TappedOutInterface::copyDeckSplitMainAndSide(DeckList &source, DeckList &mainboard, DeckList &sideboard)
-{
-    CopyMainOrSide copyMainOrSide(cardDatabase, mainboard, sideboard);
     source.forEachCard(copyMainOrSide);
 }

cockatrice/src/client/ui/picture_loader/picture_to_load.cpp

@@ -1,6 +1,7 @@
 #include "picture_to_load.h"
 
 #include "../../../settings/cache_settings.h"
+#include "../../../utility/card_set_comparator.h"
 
 #include <QCoreApplication>
 #include <QDate>
@@ -20,7 +21,7 @@ PictureToLoad::PictureToLoad(CardInfoPtr _card)
         if (sortedSets.empty()) {
             sortedSets << CardSet::newInstance("", "", "", QDate());
         }
-        std::sort(sortedSets.begin(), sortedSets.end(), SetDownloadPriorityComparator());
+        std::sort(sortedSets.begin(), sortedSets.end(), SetPriorityComparator());
 
         // If the user hasn't disabled arts other than their personal preference...
         if (!SettingsCache::instance().getOverrideAllCardArtWithPersonalPreference()) {

cockatrice/src/client/ui/picture_loader/picture_to_load.h

@@ -10,24 +10,6 @@ inline Q_LOGGING_CATEGORY(PictureToLoadLog, "picture_loader.picture_to_load");
 class PictureToLoad
 {
 private:
-    class SetDownloadPriorityComparator
-    {
-    public:
-        /*
-         * Returns true if a has higher download priority than b
-         * Enabled sets have priority over disabled sets
-         * Both groups follows the user-defined order
-         */
-        inline bool operator()(const CardSetPtr &a, const CardSetPtr &b) const
-        {
-            if (a->getEnabled()) {
-                return !b->getEnabled() || a->getSortKey() < b->getSortKey();
-            } else {
-                return !b->getEnabled() && a->getSortKey() < b->getSortKey();
-            }
-        }
-    };
-
     CardInfoPtr card;
     QList<CardSetPtr> sortedSets;
     QList<QString> urlTemplates;

cockatrice/src/deck/deck_loader.cpp

@@ -239,54 +239,33 @@ static QString getDomainForWebsite(DeckLoader::DecklistWebsite website)
     }
 }
 
-// This struct is here to support the forEachCard function call, defined in decklist. It
+/**
-// requires a function to be called for each card, and passes an inner node and a card for
+ * Converts the card to the String that represents it in the decklist export
-// each card in the decklist.
+ */
-struct FormatDeckListForExport
+static QString toDecklistExportString(const DecklistCardNode *card)
 {
-    // Create refrences for the strings that will be passed in.
+    QString cardString;
-    QString &mainBoardCards;
+    // Get the number of cards and add the card name
-    QString &sideBoardCards;
+    cardString += QString::number(card->getNumber());
-    // create main operator for struct, allowing the foreachcard to work.
+    // Add a space between card num and name
-    FormatDeckListForExport(QString &_mainBoardCards, QString &_sideBoardCards)
+    cardString += "%20";
-        : mainBoardCards(_mainBoardCards), sideBoardCards(_sideBoardCards){};
+    // Add card name
+    cardString += card->getName();
 
-    void operator()(const InnerDecklistNode *node, const DecklistCardNode *card) const
+    if (!card->getCardSetShortName().isNull()) {
-    {
+        cardString += "%20";
-        // Get the card name
+        cardString += "(" + card->getCardSetShortName() + ")";
-        CardInfoPtr dbCard = CardDatabaseManager::getInstance()->getCard(card->getName());
-        if (!dbCard || dbCard->getIsToken()) {
-            // If it's a token, we don't care about the card.
-            return;
-        }
-
-        // Check if it's a sideboard card.
-        if (node->getName() == DECK_ZONE_SIDE) {
-            // Get the number of cards and add the card name
-            sideBoardCards += QString::number(card->getNumber());
-            // Add a space between card num and name
-            sideBoardCards += "%20";
-            // Add card name
-            sideBoardCards += card->getName();
-            // Add a return at the end of the card
-            sideBoardCards += "%0A";
-        } else // If it's a mainboard card, do the same thing, but for the mainboard card string
-        {
-            mainBoardCards += QString::number(card->getNumber());
-            mainBoardCards += "%20";
-            mainBoardCards += card->getName();
-            if (!card->getCardSetShortName().isNull()) {
-                mainBoardCards += "%20";
-                mainBoardCards += "(" + card->getCardSetShortName() + ")";
-            }
-            if (!card->getCardCollectorNumber().isNull()) {
-                mainBoardCards += "%20";
-                mainBoardCards += card->getCardCollectorNumber();
-            }
-            mainBoardCards += "%0A";
-        }
     }
-};
+    if (!card->getCardCollectorNumber().isNull()) {
+        cardString += "%20";
+        cardString += card->getCardCollectorNumber();
+    }
+
+    // Add a return at the end of the card
+    cardString += "%0A";
+
+    return cardString;
+}
 
 /**
  * Export deck to decklist function, called to format the deck in a way to be sent to a server
@@ -298,8 +277,25 @@ QString DeckLoader::exportDeckToDecklist(DecklistWebsite website)
     QString deckString = "https://" + getDomainForWebsite(website) + "/?";
     // Create two strings to pass to function
     QString mainBoardCards, sideBoardCards;
-    // Set up the struct to call.
+
-    FormatDeckListForExport formatDeckListForExport(mainBoardCards, sideBoardCards);
+    // Set up the function to call
+    auto formatDeckListForExport = [&mainBoardCards, &sideBoardCards](const auto *node, const auto *card) {
+        // Get the card name
+        CardInfoPtr dbCard = CardDatabaseManager::getInstance()->getCard(card->getName());
+        if (!dbCard || dbCard->getIsToken()) {
+            // If it's a token, we don't care about the card.
+            return;
+        }
+
+        // Check if it's a sideboard card.
+        if (node->getName() == DECK_ZONE_SIDE) {
+            sideBoardCards += toDecklistExportString(card);
+        } else {
+            // If it's a mainboard card, do the same thing, but for the mainboard card string
+            mainBoardCards += toDecklistExportString(card);
+        }
+    };
+
     // call our struct function for each card in the deck
     forEachCard(formatDeckListForExport);
     // Remove the extra return at the end of the last cards
@@ -316,17 +312,12 @@ QString DeckLoader::exportDeckToDecklist(DecklistWebsite website)
     return deckString;
 }
 
-// This struct is here to support the forEachCard function call, defined in decklist.
+/**
-// It requires a function to be called for each card, and it will set the providerId.
+ * Sets the providerId on each card in the decklist based on its set name and collector number.
-struct SetProviderId
+ */
+void DeckLoader::resolveSetNameAndNumberToProviderID()
 {
-    // Main operator for struct, allowing the foreachcard to work.
+    auto setProviderId = [](const auto node, const auto card) {
-    SetProviderId()
-    {
-    }
-
-    void operator()(const InnerDecklistNode *node, DecklistCardNode *card) const
-    {
         Q_UNUSED(node);
         // Retrieve the providerId based on setName and collectorNumber
         QString providerId =
@@ -336,50 +327,23 @@ struct SetProviderId
 
         // Set the providerId on the card
         card->setCardProviderId(providerId);
-    }
+    };
-};
 
-/**
- * This function iterates through each card in the decklist and sets the providerId
- * on each card based on its set name and collector number.
- */
-void DeckLoader::resolveSetNameAndNumberToProviderID()
-{
-    // Set up the struct to call.
-    SetProviderId setProviderId;
-
-    // Call the forEachCard method for each card in the deck
     forEachCard(setProviderId);
 }
 
-// This struct is here to support the forEachCard function call, defined in decklist.
-// It requires a function to be called for each card, and it will set the providerId.
-struct ClearSetNameAndNumber
-{
-    // Main operator for struct, allowing the foreachcard to work.
-    ClearSetNameAndNumber()
-    {
-    }
-
-    void operator()(const InnerDecklistNode *node, DecklistCardNode *card) const
-    {
-        Q_UNUSED(node);
-        // Set the providerId on the card
-        card->setCardSetShortName(nullptr);
-        card->setCardCollectorNumber(nullptr);
-    }
-};
-
 /**
- * This function iterates through each card in the decklist and sets the providerId
+ * Clears the set name and numbers on each card in the decklist.
- * on each card based on its set name and collector number.
  */
 void DeckLoader::clearSetNamesAndNumbers()
 {
-    // Set up the struct to call.
+    auto clearSetNameAndNumber = [](const auto node, auto card) {
-    ClearSetNameAndNumber clearSetNameAndNumber;
+        Q_UNUSED(node)
+        // Set the providerId on the card
+        card->setCardSetShortName(nullptr);
+        card->setCardCollectorNumber(nullptr);
+    };
 
-    // Call the forEachCard method for each card in the deck
     forEachCard(clearSetNameAndNumber);
 }
 

cockatrice/src/deck/deck_stats_interface.cpp

@@ -67,26 +67,15 @@ void DeckStatsInterface::analyzeDeck(DeckList *deck)
     manager->post(request, data);
 }
 
-struct CopyIfNotAToken
+void DeckStatsInterface::copyDeckWithoutTokens(DeckList &source, DeckList &destination)
 {
-    CardDatabase &cardDatabase;
+    auto copyIfNotAToken = [this, &destination](const auto node, const auto card) {
-    DeckList &destination;
-
-    CopyIfNotAToken(CardDatabase &_cardDatabase, DeckList &_destination)
-        : cardDatabase(_cardDatabase), destination(_destination){};
-
-    void operator()(const InnerDecklistNode *node, const DecklistCardNode *card) const
-    {
         CardInfoPtr dbCard = cardDatabase.getCard(card->getName());
         if (dbCard && !dbCard->getIsToken()) {
             DecklistCardNode *addedCard = destination.addCard(card->getName(), node->getName());
             addedCard->setNumber(card->getNumber());
         }
-    }
+    };
-};
 
-void DeckStatsInterface::copyDeckWithoutTokens(DeckList &source, DeckList &destination)
-{
-    CopyIfNotAToken copyIfNotAToken(cardDatabase, destination);
     source.forEachCard(copyIfNotAToken);
 }

common/decklist.cpp

@@ -262,21 +262,6 @@ bool AbstractDecklistCardNode::compareName(AbstractDecklistNode *other) const
     }
 }
 
-class InnerDecklistNode::compareFunctor
-{
-private:
-    Qt::SortOrder order;
-
-public:
-    explicit compareFunctor(Qt::SortOrder _order) : order(_order)
-    {
-    }
-    inline bool operator()(QPair<int, AbstractDecklistNode *> a, QPair<int, AbstractDecklistNode *> b) const
-    {
-        return (order == Qt::AscendingOrder) ? (b.second->compare(a.second)) : (a.second->compare(b.second));
-    }
-};
-
 bool InnerDecklistNode::readElement(QXmlStreamReader *xml)
 {
     while (!xml->atEnd()) {
@@ -347,7 +332,10 @@ QVector<QPair<int, int>> InnerDecklistNode::sort(Qt::SortOrder order)
     }
 
     // Sort temporary list
-    compareFunctor cmp(order);
+    auto cmp = [order](const auto &a, const auto &b) {
+        return (order == Qt::AscendingOrder) ? (b.second->compare(a.second)) : (a.second->compare(b.second));
+    };
+
     std::sort(tempList.begin(), tempList.end(), cmp);
 
     // Map old indexes to new indexes and
@@ -762,20 +750,9 @@ bool DeckList::loadFromFile_Plain(QIODevice *device)
     return loadFromStream_Plain(in, false);
 }
 
-struct WriteToStream
+bool DeckList::saveToStream_Plain(QTextStream &stream, bool prefixSideboardCards, bool slashTappedOutSplitCards)
 {
-    QTextStream &stream;
+    auto writeToStream = [&stream, prefixSideboardCards, slashTappedOutSplitCards](const auto node, const auto card) {
-    bool prefixSideboardCards;
-    bool slashTappedOutSplitCards;
-
-    WriteToStream(QTextStream &_stream, bool _prefixSideboardCards, bool _slashTappedOutSplitCards)
-        : stream(_stream), prefixSideboardCards(_prefixSideboardCards),
-          slashTappedOutSplitCards(_slashTappedOutSplitCards)
-    {
-    }
-
-    void operator()(const InnerDecklistNode *node, const DecklistCardNode *card)
-    {
         if (prefixSideboardCards && node->getName() == DECK_ZONE_SIDE) {
             stream << "SB: ";
         }
@@ -784,12 +761,8 @@ struct WriteToStream
         } else {
             stream << QString("%1 %2\n").arg(card->getNumber()).arg(card->getName().replace("//", "/"));
         }
-    }
+    };
-};
 
-bool DeckList::saveToStream_Plain(QTextStream &out, bool prefixSideboardCards, bool slashTappedOutSplitCards)
-{
-    WriteToStream writeToStream(out, prefixSideboardCards, slashTappedOutSplitCards);
     forEachCard(writeToStream);
     return true;
 }
@@ -994,3 +967,19 @@ void DeckList::refreshDeckHash()
     cachedDeckHash = QString();
     emit deckHashChanged();
 }
+
+/**
+ * Calls a given function on each card in the deck.
+ */
+void DeckList::forEachCard(const std::function<void(InnerDecklistNode *, DecklistCardNode *)> &func)
+{
+    // Support for this is only possible if the internal structure
+    // doesn't get more complicated.
+    for (int i = 0; i < root->size(); i++) {
+        InnerDecklistNode *node = dynamic_cast<InnerDecklistNode *>(root->at(i));
+        for (int j = 0; j < node->size(); j++) {
+            DecklistCardNode *card = dynamic_cast<DecklistCardNode *>(node->at(j));
+            func(node, card);
+        }
+    }
+}

common/decklist.h

@@ -381,23 +381,7 @@ public:
     QString getDeckHash() const;
     void refreshDeckHash();
 
-    /**
+    void forEachCard(const std::function<void(InnerDecklistNode *, DecklistCardNode *)> &func);
-     * Calls a given function object for each card in the deck. It must
-     * take a InnerDecklistNode* as its first argument and a
-     * DecklistCardNode* as its second.
-     */
-    template <typename Callback> void forEachCard(Callback &callback)
-    {
-        // Support for this is only possible if the internal structure
-        // doesn't get more complicated.
-        for (int i = 0; i < root->size(); i++) {
-            InnerDecklistNode *node = dynamic_cast<InnerDecklistNode *>(root->at(i));
-            for (int j = 0; j < node->size(); j++) {
-                DecklistCardNode *card = dynamic_cast<DecklistCardNode *>(node->at(j));
-                callback(node, card);
-            }
-        }
-    }
 };
 
 #endif

tests/loading_from_clipboard/clipboard_testing.cpp

@@ -2,17 +2,6 @@
 
 #include <QTextStream>
 
-void Result::operator()(const InnerDecklistNode *innerDecklistNode, const DecklistCardNode *card)
-{
-    if (innerDecklistNode->getName() == DECK_ZONE_MAIN) {
-        mainboard.append({card->getName().toStdString(), card->getNumber()});
-    } else if (innerDecklistNode->getName() == DECK_ZONE_SIDE) {
-        sideboard.append({card->getName().toStdString(), card->getNumber()});
-    } else {
-        FAIL();
-    }
-}
-
 void testEmpty(const QString &clipboard)
 {
     QString cp(clipboard);
@@ -33,9 +22,22 @@ void testDeck(const QString &clipboard, const Result &result)
     ASSERT_EQ(result.name, deckList.getName().toStdString());
     ASSERT_EQ(result.comments, deckList.getComments().toStdString());
 
-    Result decklistBuilder;
+    CardRows mainboard;
-    deckList.forEachCard(decklistBuilder);
+    CardRows sideboard;
 
-    ASSERT_EQ(result.mainboard, decklistBuilder.mainboard);
+    auto extractCards = [&mainboard, &sideboard](const InnerDecklistNode *innerDecklistNode,
-    ASSERT_EQ(result.sideboard, decklistBuilder.sideboard);
+                                                 const DecklistCardNode *card) {
+        if (innerDecklistNode->getName() == DECK_ZONE_MAIN) {
+            mainboard.append({card->getName().toStdString(), card->getNumber()});
+        } else if (innerDecklistNode->getName() == DECK_ZONE_SIDE) {
+            sideboard.append({card->getName().toStdString(), card->getNumber()});
+        } else {
+            FAIL();
+        }
+    };
+
+    deckList.forEachCard(extractCards);
+
+    ASSERT_EQ(result.mainboard, mainboard);
+    ASSERT_EQ(result.sideboard, sideboard);
 }

tests/loading_from_clipboard/clipboard_testing.h

@@ -5,25 +5,20 @@
 
 #include "gtest/gtest.h"
 
+// using std types because qt types aren't understood by gtest (without this you'll get less nice errors)
+using CardRows = QVector<std::pair<std::string, int>>;
+
 struct Result
 {
-    // using std types because qt types aren't understood by gtest (without this you'll get less nice errors)
-    using CardRows = QVector<std::pair<std::string, int>>;
     std::string name;
     std::string comments;
     CardRows mainboard;
     CardRows sideboard;
 
-    Result()
-    {
-    }
-
     Result(std::string _name, std::string _comments, CardRows _mainboard, CardRows _sideboard)
         : name(_name), comments(_comments), mainboard(_mainboard), sideboard(_sideboard)
     {
     }
-
-    void operator()(const InnerDecklistNode *innerDecklistNode, const DecklistCardNode *card);
 };
 
 void testEmpty(const QString &clipboard);