Refactor: clean up OracleImporter (#5871)

* remove unused dataDir variable

* inline setsMap

* join declaration and assignment

* make the protected methods static

* make getSetPriority static

* inline mainCardTypes list and make the method static

* pass by const ref when able

* rename param to match

oracle/src/oracleimporter.cpp

@@ -12,18 +12,18 @@
 SplitCardPart::SplitCardPart(const QString &_name,
                              const QString &_text,
                              const QVariantHash &_properties,
-                             const CardInfoPerSet _setInfo)
+                             const CardInfoPerSet &_setInfo)
     : name(_name), text(_text), properties(_properties), setInfo(_setInfo)
 {
 }
 
 const QRegularExpression OracleImporter::formatRegex = QRegularExpression("^format-");
 
-OracleImporter::OracleImporter(const QString &_dataDir, QObject *parent) : QObject(parent), dataDir(_dataDir)
+OracleImporter::OracleImporter(QObject *parent) : QObject(parent)
 {
 }
 
-CardSet::Priority OracleImporter::getSetPriority(QString &setType, QString &shortName)
+static CardSet::Priority getSetPriority(const QString &setType, const QString &shortName)
 {
     if (!setTypePriorities.contains(setType.toLower())) {
         qDebug() << "warning: Set type" << setType << "unrecognized for prioritization";
@@ -40,30 +40,22 @@ bool OracleImporter::readSetsFromByteArray(const QByteArray &data)
     QList<SetToDownload> newSetList;
 
     bool ok;
-    setsMap = QtJson::Json::parse(QString(data), ok).toMap().value("data").toMap();
+    auto setsMap = QtJson::Json::parse(QString(data), ok).toMap().value("data").toMap();
     if (!ok) {
         qDebug() << "error: QtJson::Json::parse()";
         return false;
     }
 
     QListIterator it(setsMap.values());
-    QVariantMap map;
-
-    QString shortName;
-    QString longName;
-    QList<QVariant> setCards;
-    QString setType;
-    QDate releaseDate;
-    CardSet::Priority priority;
 
     while (it.hasNext()) {
-        map = it.next().toMap();
+        QVariantMap map = it.next().toMap();
-        shortName = map.value("code").toString().toUpper();
+        QString shortName = map.value("code").toString().toUpper();
-        longName = map.value("name").toString();
+        QString longName = map.value("name").toString();
-        setCards = map.value("cards").toList();
+        QList<QVariant> setCards = map.value("cards").toList();
-        setType = map.value("type").toString();
+        QString setType = map.value("type").toString();
-        releaseDate = map.value("releaseDate").toDate();
+        QDate releaseDate = map.value("releaseDate").toDate();
-        priority = getSetPriority(setType, shortName);
+        CardSet::Priority priority = getSetPriority(setType, shortName);
         // capitalize set type
         if (setType.length() > 0) {
             // basic grammar for words that aren't capitalized, like in "From the Vault"
@@ -93,13 +85,16 @@ bool OracleImporter::readSetsFromByteArray(const QByteArray &data)
     return true;
 }
 
-QString OracleImporter::getMainCardType(const QStringList &typeList)
+static QString getMainCardType(const QStringList &typeList)
 {
     if (typeList.isEmpty()) {
         return {};
     }
 
-    for (const auto &type : mainCardTypes) {
+    static const QStringList typePriority = {"Planeswalker", "Creature", "Land",       "Sorcery",
+                                             "Instant",      "Artifact", "Enchantment"};
+
+    for (const auto &type : typePriority) {
         if (typeList.contains(type)) {
             return type;
         }
@@ -108,12 +103,33 @@ QString OracleImporter::getMainCardType(const QStringList &typeList)
     return typeList.first();
 }
 
+/**
+ * Sorts and deduplicates the color chars in the string by WUBRG order.
+ *
+ * @param colors The string containing the color chars. Will be modified in-place
+ */
+static void sortAndReduceColors(QString &colors)
+{
+    // sort
+    static const QHash<QChar, unsigned int> colorOrder{{'W', 0}, {'U', 1}, {'B', 2}, {'R', 3}, {'G', 4}};
+    std::sort(colors.begin(), colors.end(),
+              [](const QChar a, const QChar b) { return colorOrder.value(a, INT_MAX) < colorOrder.value(b, INT_MAX); });
+    // reduce
+    QChar lastChar = '\0';
+    for (int i = 0; i < colors.size(); ++i) {
+        if (colors.at(i) == lastChar)
+            colors.remove(i, 1);
+        else
+            lastChar = colors.at(i);
+    }
+}
+
 CardInfoPtr OracleImporter::addCard(QString name,
-                                    QString text,
+                                    const QString &text,
                                     bool isToken,
                                     QVariantHash properties,
-                                    QList<CardRelation *> &relatedCards,
+                                    const QList<CardRelation *> &relatedCards,
-                                    CardInfoPerSet setInfo)
+                                    const CardInfoPerSet &setInfo)
 {
     // Workaround for card name weirdness
     name = name.replace("Æ", "AE");
@@ -197,7 +213,7 @@ CardInfoPtr OracleImporter::addCard(QString name,
     return newCard;
 }
 
-QString OracleImporter::getStringPropertyFromMap(const QVariantMap &card, const QString &propertyName)
+static QString getStringPropertyFromMap(const QVariantMap &card, const QString &propertyName)
 {
     return card.contains(propertyName) ? card.value(propertyName).toString() : QString("");
 }
@@ -442,23 +458,6 @@ int OracleImporter::importCardsFromSet(const CardSetPtr &currentSet, const QList
     return numCards;
 }
 
-void OracleImporter::sortAndReduceColors(QString &colors)
-{
-    // sort
-    const QHash<QChar, unsigned int> colorOrder{{'W', 0}, {'U', 1}, {'B', 2}, {'R', 3}, {'G', 4}};
-    std::sort(colors.begin(), colors.end(), [&colorOrder](const QChar a, const QChar b) {
-        return colorOrder.value(a, INT_MAX) < colorOrder.value(b, INT_MAX);
-    });
-    // reduce
-    QChar lastChar = '\0';
-    for (int i = 0; i < colors.size(); ++i) {
-        if (colors.at(i) == lastChar)
-            colors.remove(i, 1);
-        else
-            lastChar = colors.at(i);
-    }
-}
-
 int OracleImporter::startImport()
 {
     int setCards = 0, setIndex = 0;

oracle/src/oracleimporter.h

@@ -92,7 +92,10 @@ public:
 class SplitCardPart
 {
 public:
-    SplitCardPart(const QString &_name, const QString &_text, const QVariantHash &_properties, CardInfoPerSet setInfo);
+    SplitCardPart(const QString &_name,
+                  const QString &_text,
+                  const QVariantHash &_properties,
+                  const CardInfoPerSet &setInfo);
     inline const QString &getName() const
     {
         return name;
@@ -121,8 +124,6 @@ class OracleImporter : public QObject
 {
     Q_OBJECT
 private:
-    const QStringList mainCardTypes = {"Planeswalker", "Creature", "Land",       "Sorcery",
-                                       "Instant",      "Artifact", "Enchantment"};
     static const QRegularExpression formatRegex;
 
     /**
@@ -136,27 +137,23 @@ private:
     SetNameMap sets;
 
     QList<SetToDownload> allSets;
-    QVariantMap setsMap;
-    QString dataDir;
 
-    QString getMainCardType(const QStringList &typeList);
     CardInfoPtr addCard(QString name,
-                        QString text,
+                        const QString &text,
                         bool isToken,
                         QVariantHash properties,
-                        QList<CardRelation *> &relatedCards,
+                        const QList<CardRelation *> &relatedCards,
-                        CardInfoPerSet setInfo);
+                        const CardInfoPerSet &setInfo);
 signals:
     void setIndexChanged(int cardsImported, int setIndex, const QString &setName);
     void dataReadProgress(int bytesRead, int totalBytes);
 
 public:
-    explicit OracleImporter(const QString &_dataDir, QObject *parent = nullptr);
+    explicit OracleImporter(QObject *parent = nullptr);
-    CardSet::Priority getSetPriority(QString &setType, QString &shortName);
     bool readSetsFromByteArray(const QByteArray &data);
     int startImport();
     bool saveToFile(const QString &fileName, const QString &sourceUrl, const QString &sourceVersion);
-    int importCardsFromSet(const CardSetPtr &currentSet, const QList<QVariant> &cards);
+    int importCardsFromSet(const CardSetPtr &currentSet, const QList<QVariant> &cardsList);
     const CardNameMap &getCardList() const
     {
         return cards;
@@ -165,15 +162,7 @@ public:
     {
         return allSets;
     }
-    const QString &getDataDir() const
-    {
-        return dataDir;
-    }
     void clear();
-
-protected:
-    inline QString getStringPropertyFromMap(const QVariantMap &card, const QString &propertyName);
-    void sortAndReduceColors(QString &colors);
 };
 
 #endif

oracle/src/oraclewizard.cpp

@@ -60,7 +60,7 @@ OracleWizard::OracleWizard(QWidget *parent) : QWizard(parent)
     settings = new QSettings(SettingsCache::instance().getSettingsPath() + "global.ini", QSettings::IniFormat, this);
     connect(&SettingsCache::instance(), &SettingsCache::langChanged, this, &OracleWizard::updateLanguage);
 
-    importer = new OracleImporter(SettingsCache::instance().getDataPath(), this);
+    importer = new OracleImporter(this);
 
     nam = new QNetworkAccessManager(this);