From 26dcb015cec1592e2e6bb74575c58f1349c2bf8e Mon Sep 17 00:00:00 2001 From: RickyRister <42636155+RickyRister@users.noreply.github.com> Date: Sat, 19 Apr 2025 21:07:22 -0700 Subject: [PATCH] Refactor function structs into lambdas (#5675) * change signature to use lambda * reuse comparator * inline structs in forEachCard * inline structs * Refactor exportDeckToDecklist * fix unit test --- .../src/client/tapped_out_interface.cpp | 17 +- .../ui/picture_loader/picture_to_load.cpp | 3 +- .../ui/picture_loader/picture_to_load.h | 18 --- cockatrice/src/deck/deck_loader.cpp | 148 +++++++----------- cockatrice/src/deck/deck_stats_interface.cpp | 17 +- common/decklist.cpp | 57 +++---- common/decklist.h | 18 +-- .../clipboard_testing.cpp | 32 ++-- .../clipboard_testing.h | 11 +- 9 files changed, 108 insertions(+), 213 deletions(-) diff --git a/cockatrice/src/client/tapped_out_interface.cpp b/cockatrice/src/client/tapped_out_interface.cpp index d6a47b018..4fd103c8c 100644 --- a/cockatrice/src/client/tapped_out_interface.cpp +++ b/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; - 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 - { + auto copyMainOrSide = [this, &mainboard, &sideboard](const auto node, const auto card) { 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); } diff --git a/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp b/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp index 9800d7cc5..81210a983 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_to_load.cpp +++ b/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 #include @@ -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()) { diff --git a/cockatrice/src/client/ui/picture_loader/picture_to_load.h b/cockatrice/src/client/ui/picture_loader/picture_to_load.h index 9d1a784f0..38ebe5763 100644 --- a/cockatrice/src/client/ui/picture_loader/picture_to_load.h +++ b/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 sortedSets; QList urlTemplates; diff --git a/cockatrice/src/deck/deck_loader.cpp b/cockatrice/src/deck/deck_loader.cpp index 1c7136bf5..d0c0f5239 100644 --- a/cockatrice/src/deck/deck_loader.cpp +++ b/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 -// each card in the decklist. -struct FormatDeckListForExport +/** + * Converts the card to the String that represents it in the decklist export + */ +static QString toDecklistExportString(const DecklistCardNode *card) { - // Create refrences for the strings that will be passed in. - QString &mainBoardCards; - QString &sideBoardCards; - // create main operator for struct, allowing the foreachcard to work. - FormatDeckListForExport(QString &_mainBoardCards, QString &_sideBoardCards) - : mainBoardCards(_mainBoardCards), sideBoardCards(_sideBoardCards){}; + QString cardString; + // Get the number of cards and add the card name + cardString += QString::number(card->getNumber()); + // Add a space between card num and name + cardString += "%20"; + // Add card name + cardString += card->getName(); - void operator()(const InnerDecklistNode *node, const DecklistCardNode *card) const - { - // 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) { - // 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->getCardSetShortName().isNull()) { + cardString += "%20"; + cardString += "(" + card->getCardSetShortName() + ")"; } -}; + 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. -struct SetProviderId +/** + * Sets the providerId on each card in the decklist based on its set name and collector number. + */ +void DeckLoader::resolveSetNameAndNumberToProviderID() { - // Main operator for struct, allowing the foreachcard to work. - SetProviderId() - { - } - - void operator()(const InnerDecklistNode *node, DecklistCardNode *card) const - { + auto setProviderId = [](const auto node, const auto card) { 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 - * on each card based on its set name and collector number. + * Clears the set name and numbers on each card in the decklist. */ void DeckLoader::clearSetNamesAndNumbers() { - // Set up the struct to call. - ClearSetNameAndNumber clearSetNameAndNumber; + auto clearSetNameAndNumber = [](const auto node, auto card) { + 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); } diff --git a/cockatrice/src/deck/deck_stats_interface.cpp b/cockatrice/src/deck/deck_stats_interface.cpp index dc2ab14df..7cfd69d40 100644 --- a/cockatrice/src/deck/deck_stats_interface.cpp +++ b/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; - DeckList &destination; - - CopyIfNotAToken(CardDatabase &_cardDatabase, DeckList &_destination) - : cardDatabase(_cardDatabase), destination(_destination){}; - - void operator()(const InnerDecklistNode *node, const DecklistCardNode *card) const - { + auto copyIfNotAToken = [this, &destination](const auto node, const auto card) { 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); } diff --git a/common/decklist.cpp b/common/decklist.cpp index 789f57910..6f27d2895 100644 --- a/common/decklist.cpp +++ b/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 a, QPair 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> 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; - 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) - { + auto writeToStream = [&stream, prefixSideboardCards, slashTappedOutSplitCards](const auto node, const auto 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 &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(root->at(i)); + for (int j = 0; j < node->size(); j++) { + DecklistCardNode *card = dynamic_cast(node->at(j)); + func(node, card); + } + } +} \ No newline at end of file diff --git a/common/decklist.h b/common/decklist.h index de839ad23..989c3e7fe 100644 --- a/common/decklist.h +++ b/common/decklist.h @@ -381,23 +381,7 @@ public: QString getDeckHash() const; void refreshDeckHash(); - /** - * 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 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(root->at(i)); - for (int j = 0; j < node->size(); j++) { - DecklistCardNode *card = dynamic_cast(node->at(j)); - callback(node, card); - } - } - } + void forEachCard(const std::function &func); }; #endif \ No newline at end of file diff --git a/tests/loading_from_clipboard/clipboard_testing.cpp b/tests/loading_from_clipboard/clipboard_testing.cpp index ad8db3dbf..b2021bde5 100644 --- a/tests/loading_from_clipboard/clipboard_testing.cpp +++ b/tests/loading_from_clipboard/clipboard_testing.cpp @@ -2,17 +2,6 @@ #include -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; - deckList.forEachCard(decklistBuilder); + CardRows mainboard; + CardRows sideboard; - ASSERT_EQ(result.mainboard, decklistBuilder.mainboard); - ASSERT_EQ(result.sideboard, decklistBuilder.sideboard); + auto extractCards = [&mainboard, &sideboard](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(); + } + }; + + deckList.forEachCard(extractCards); + + ASSERT_EQ(result.mainboard, mainboard); + ASSERT_EQ(result.sideboard, sideboard); } diff --git a/tests/loading_from_clipboard/clipboard_testing.h b/tests/loading_from_clipboard/clipboard_testing.h index 272801126..00d73b636 100644 --- a/tests/loading_from_clipboard/clipboard_testing.h +++ b/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>; + 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::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);