Manage CharacterData using std::unique_ptr

Fixes many memory leaks, but also made it clear that we're very often
loading all the character data only to immediately throw it away again,
even when most of the time all we really need is the database ID or the
name.

1 files changed, 71 insertions, 72 deletions

diff --git a/src/account-server/storage.cpp b/src/account-server/storage.cpp
index 96590524..9ff25f69 100644
--- a/src/account-server/storage.cpp
+++ b/src/account-server/storage.cpp
@@ -222,10 +222,9 @@ std::unique_ptr<Account> Storage::getAccountBySQL()
 
             for (int k = 0; k < size; ++k)
             {
-                if (CharacterData *ptr =
-                        getCharacter(characterIDs[k], account.get()))
+                if (auto character = getCharacter(characterIDs[k], account.get()))
                 {
-                    characters[ptr->getCharacterSlot()] = ptr;
+                    characters[character->getCharacterSlot()] = std::move(character);
                 }
                 else
                 {
@@ -234,7 +233,7 @@ std::unique_ptr<Account> Storage::getAccountBySQL()
                 }
             }
 
-            account->setCharacters(characters);
+            account->setCharacters(std::move(characters));
         }
 
         return account;
@@ -337,9 +336,9 @@ std::unique_ptr<Account> Storage::getAccount(int accountID)
     return nullptr;
 }
 
-CharacterData *Storage::getCharacterBySQL(Account *owner)
+std::unique_ptr<CharacterData> Storage::getCharacterBySQL(Account *owner)
 {
-    CharacterData *character = nullptr;
+    std::unique_ptr<CharacterData> character = nullptr;
 
     string_to< unsigned > toUint;
     string_to< int > toInt;
@@ -356,7 +355,7 @@ CharacterData *Storage::getCharacterBySQL(Account *owner)
         string_to< unsigned short > toUshort;
         string_to< double > toDouble;
 
-        character = new CharacterData(charInfo(0, 2), toUint(charInfo(0, 0)));
+        character = std::make_unique<CharacterData>(charInfo(0, 2), toUint(charInfo(0, 0)));
         character->setGender(toUshort(charInfo(0, 3)));
         character->setHairStyle(toUshort(charInfo(0, 4)));
         character->setHairColor(toUshort(charInfo(0, 5)));
@@ -527,7 +526,7 @@ CharacterData *Storage::getCharacterBySQL(Account *owner)
     return character;
 }
 
-CharacterData *Storage::getCharacter(int id, Account *owner)
+std::unique_ptr<CharacterData> Storage::getCharacter(int id, Account *owner)
 {
     std::ostringstream sql;
     sql << "SELECT * FROM " << CHARACTERS_TBL_NAME << " WHERE id = ?";
@@ -539,7 +538,7 @@ CharacterData *Storage::getCharacter(int id, Account *owner)
     return nullptr;
 }
 
-CharacterData *Storage::getCharacter(const std::string &name)
+std::unique_ptr<CharacterData> Storage::getCharacter(const std::string &name)
 {
     std::ostringstream sql;
     sql << "SELECT * FROM " << CHARACTERS_TBL_NAME << " WHERE name = ?";
@@ -676,7 +675,7 @@ bool Storage::doesCharacterNameExist(const std::string& name)
     return true;
 }
 
-bool Storage::updateCharacter(CharacterData *character)
+bool Storage::updateCharacter(const CharacterData &character)
 {
     dal::PerformTransaction transaction(mDb);
 
@@ -687,16 +686,16 @@ bool Storage::updateCharacter(CharacterData *character)
         sqlUpdateCharacterInfo
             << "update "        << CHARACTERS_TBL_NAME << " "
             << "set "
-            << "gender = '"     << character->getGender() << "', "
-            << "hair_style = '" << character->getHairStyle() << "', "
-            << "hair_color = '" << character->getHairColor() << "', "
-            << "char_pts = '"   << character->getAttributePoints() << "', "
-            << "correct_pts = '"<< character->getCorrectionPoints() << "', "
-            << "x = '"          << character->getPosition().x << "', "
-            << "y = '"          << character->getPosition().y << "', "
-            << "map_id = '"     << character->getMapId() << "', "
-            << "slot = '"     << character->getCharacterSlot() << "' "
-            << "where id = '"   << character->getDatabaseID() << "';";
+            << "gender = '"     << character.getGender() << "', "
+            << "hair_style = '" << character.getHairStyle() << "', "
+            << "hair_color = '" << character.getHairColor() << "', "
+            << "char_pts = '"   << character.getAttributePoints() << "', "
+            << "correct_pts = '"<< character.getCorrectionPoints() << "', "
+            << "x = '"          << character.getPosition().x << "', "
+            << "y = '"          << character.getPosition().y << "', "
+            << "map_id = '"     << character.getMapId() << "', "
+            << "slot = '"     << character.getCharacterSlot() << "' "
+            << "where id = '"   << character.getDatabaseID() << "';";
 
         mDb->execSql(sqlUpdateCharacterInfo.str());
     }
@@ -709,9 +708,9 @@ bool Storage::updateCharacter(CharacterData *character)
     // Character attributes.
     try
     {
-        for (AttributeMap::const_iterator it = character->mAttributes.begin(),
-             it_end = character->mAttributes.end(); it != it_end; ++it)
-            updateAttribute(character->getDatabaseID(), it->first,
+        for (AttributeMap::const_iterator it = character.mAttributes.begin(),
+             it_end = character.mAttributes.end(); it != it_end; ++it)
+            updateAttribute(character.getDatabaseID(), it->first,
                             it->second.base, it->second.modified);
     }
     catch (const dal::DbSqlQueryExecFailure &e)
@@ -724,10 +723,10 @@ bool Storage::updateCharacter(CharacterData *character)
     try
     {
         std::map<int, int>::const_iterator kill_it;
-        for (kill_it = character->getKillCountBegin();
-             kill_it != character->getKillCountEnd(); ++kill_it)
+        for (kill_it = character.getKillCountBegin();
+             kill_it != character.getKillCountEnd(); ++kill_it)
         {
-            updateKillCount(character->getDatabaseID(),
+            updateKillCount(character.getDatabaseID(),
                             kill_it->first, kill_it->second);
         }
     }
@@ -745,16 +744,16 @@ bool Storage::updateCharacter(CharacterData *character)
         std::ostringstream insertSql;
         deleteSql   << "DELETE FROM " << CHAR_ABILITIES_TBL_NAME
                     << " WHERE char_id='"
-                    << character->getDatabaseID() << "';";
+                    << character.getDatabaseID() << "';";
         mDb->execSql(deleteSql.str());
         // In with the new
-        for (int abilityId : character->getAbilities())
+        for (int abilityId : character.getAbilities())
         {
             insertSql.str("");
             insertSql   << "INSERT INTO " << CHAR_ABILITIES_TBL_NAME
                         << " (char_id, ability_id)"
                         << " VALUES ("
-                        << " '" << character->getDatabaseID() << "',"
+                        << " '" << character.getDatabaseID() << "',"
                         << " '" << abilityId
                         << "');";
             mDb->execSql(insertSql.str());
@@ -774,17 +773,17 @@ bool Storage::updateCharacter(CharacterData *character)
         std::ostringstream insertSql;
         deleteSql   << "DELETE FROM " << QUESTLOG_TBL_NAME
                     << " WHERE char_id='"
-                    << character->getDatabaseID() << "';";
+                    << character.getDatabaseID() << "';";
         mDb->execSql(deleteSql.str());
         // In with the new
-        for (QuestInfo &quest : character->mQuests)
+        for (const QuestInfo &quest : character.mQuests)
         {
             insertSql.str("");
             insertSql   << "INSERT INTO " << QUESTLOG_TBL_NAME
                         << " (char_id, quest_id, quest_state, "
                         << "quest_title, quest_description)"
                         << " VALUES ("
-                        << character->getDatabaseID() << ","
+                        << character.getDatabaseID() << ","
                         << " " << quest.id << ","
                         << " " << quest.state << ","
                         << " ?,"
@@ -812,7 +811,7 @@ bool Storage::updateCharacter(CharacterData *character)
         std::ostringstream sqlDeleteCharacterInventory;
         sqlDeleteCharacterInventory
             << "delete from " << INVENTORIES_TBL_NAME
-            << " where owner_id = '" << character->getDatabaseID() << "';";
+            << " where owner_id = '" << character.getDatabaseID() << "';";
         mDb->execSql(sqlDeleteCharacterInventory.str());
     }
     catch (const dal::DbSqlQueryExecFailure& e)
@@ -828,10 +827,10 @@ bool Storage::updateCharacter(CharacterData *character)
 
         sql << "insert into " << INVENTORIES_TBL_NAME
             << " (owner_id, slot, class_id, amount, equipped) values ("
-            << character->getDatabaseID() << ", ";
+            << character.getDatabaseID() << ", ";
         std::string base = sql.str();
 
-        const Possessions &poss = character->getPossessions();
+        const Possessions &poss = character.getPossessions();
         const InventoryData &inventoryData = poss.getInventory();
         for (const auto &itemIt : inventoryData)
         {
@@ -860,7 +859,7 @@ bool Storage::updateCharacter(CharacterData *character)
         std::ostringstream sql;
 
         sql << "delete from " << CHAR_STATUS_EFFECTS_TBL_NAME
-            << " where char_id = '" << character->getDatabaseID() << "';";
+            << " where char_id = '" << character.getDatabaseID() << "';";
 
          mDb->execSql(sql.str());
     }
@@ -872,10 +871,10 @@ bool Storage::updateCharacter(CharacterData *character)
     try
     {
         std::map<int, Status>::const_iterator status_it;
-        for (status_it = character->getStatusEffectBegin();
-             status_it != character->getStatusEffectEnd(); ++status_it)
+        for (status_it = character.getStatusEffectBegin();
+             status_it != character.getStatusEffectEnd(); ++status_it)
         {
-            insertStatusEffect(character->getDatabaseID(),
+            insertStatusEffect(character.getDatabaseID(),
                                status_it->first, status_it->second.time);
         }
     }
@@ -928,7 +927,7 @@ void Storage::addAccount(Account &account)
     }
 }
 
-void Storage::flush(const Account &account)
+void Storage::flush(Account &account)
 {
     assert(account.getID() >= 0);
 
@@ -963,13 +962,14 @@ void Storage::flush(const Account &account)
         }
 
         // Get the list of characters that belong to this account.
-        const Characters &characters = account.getCharacters();
+        Characters &characters = account.getCharacters();
 
         // Insert or update the characters.
-        for (auto it : characters)
+        for (auto& it : characters)
         {
-            CharacterData *character = it.second;
-            if (character->getDatabaseID() >= 0)
+            CharacterData &character = *it.second;
+
+            if (character.getDatabaseID() >= 0)
             {
                 updateCharacter(character);
             }
@@ -985,31 +985,31 @@ void Storage::flush(const Account &account)
                      << " char_pts, correct_pts,"
                      << " x, y, map_id, slot) values ("
                      << account.getID() << ", ?, "
-                     << character->getGender() << ", "
-                     << character->getHairStyle() << ", "
-                     << character->getHairColor() << ", "
-                     << character->getAttributePoints() << ", "
-                     << character->getCorrectionPoints() << ", "
-                     << character->getPosition().x << ", "
-                     << character->getPosition().y << ", "
-                     << character->getMapId() << ", "
-                     << character->getCharacterSlot()
+                     << character.getGender() << ", "
+                     << character.getHairStyle() << ", "
+                     << character.getHairColor() << ", "
+                     << character.getAttributePoints() << ", "
+                     << character.getCorrectionPoints() << ", "
+                     << character.getPosition().x << ", "
+                     << character.getPosition().y << ", "
+                     << character.getMapId() << ", "
+                     << character.getCharacterSlot()
                      << ");";
 
                 mDb->prepareSql(sqlInsertCharactersTable.str());
-                mDb->bindValue(1, character->getName());
+                mDb->bindValue(1, character.getName());
                 mDb->processSql();
 
                 // Update the character ID.
-                character->setDatabaseID(mDb->getLastId());
+                character.setDatabaseID(mDb->getLastId());
 
                 // Update all attributes.
                 AttributeMap::const_iterator attr_it, attr_end;
-                for (attr_it =  character->mAttributes.begin(),
-                     attr_end = character->mAttributes.end();
+                for (attr_it =  character.mAttributes.begin(),
+                     attr_end = character.mAttributes.end();
                      attr_it != attr_end; ++attr_it)
                 {
-                    updateAttribute(character->getDatabaseID(), attr_it->first,
+                    updateAttribute(character.getDatabaseID(), attr_it->first,
                                     attr_it->second.base,
                                     attr_it->second.modified);
                 }
@@ -1036,7 +1036,7 @@ void Storage::flush(const Account &account)
         for (unsigned i = 0; i < charInMemInfo.rows(); ++i) // In database
         {
             charFound = false;
-            for (auto character : characters) // In memory
+            for (auto& character : characters) // In memory
             {
                 if (charInMemInfo(i, 0) == character.second->getName())
                 {
@@ -1427,12 +1427,14 @@ std::map<int, Guild*> Storage::getGuildList()
         string_to< unsigned > toUint;
 
         // Add the members to the guilds.
-        for (auto &guild : guilds)
+        for (auto &idAndGuild : guilds)
         {
+            const Guild &guild = *idAndGuild.second;
+
             std::ostringstream memberSql;
             memberSql << "select member_id, rights from "
                       << GUILD_MEMBERS_TBL_NAME
-                      << " where guild_id = '" << guild.second->getId() << "';";
+                      << " where guild_id = '" << guild.getId() << "';";
             const dal::RecordSet& memberInfo = mDb->execSql(memberSql.str());
 
             std::list<std::pair<int, int> > members;
@@ -1444,10 +1446,10 @@ std::map<int, Guild*> Storage::getGuildList()
 
             for (auto i : members)
             {
-                if (CharacterData *character = getCharacter(i.first, nullptr))
+                if (auto character = getCharacter(i.first, nullptr))
                 {
-                    character->addGuild(guild.second->getName());
-                    guild.second->addMember(character->getDatabaseID(), i.second);
+                    character->addGuild(guild.getName());
+                    idAndGuild.second->addMember(character->getDatabaseID(), i.second);
                 }
             }
         }
@@ -1739,11 +1741,6 @@ void Storage::delCharacter(int charId) const
     }
 }
 
-void Storage::delCharacter(CharacterData *character) const
-{
-    delCharacter(character->getDatabaseID());
-}
-
 void Storage::checkBannedAccounts()
 {
     try
@@ -1877,10 +1874,12 @@ Post *Storage::getStoredPost(int playerId)
         for (unsigned i = 0; i < post.rows(); i++ )
         {
             // Load sender and receiver
-            CharacterData *sender = getCharacter(toUint(post(i, 1)), nullptr);
-            CharacterData *receiver = getCharacter(toUint(post(i, 2)), nullptr);
+            auto sender = getCharacter(toUint(post(i, 1)), nullptr);
+            auto receiver = getCharacter(toUint(post(i, 2)), nullptr);
 
-            auto letter = new Letter(toUint( post(0,3) ), sender, receiver);
+            auto letter = new Letter(toUint( post(0,3) ),
+                                     std::move(sender),
+                                     std::move(receiver));
 
             letter->setId( toUint(post(0, 0)) );
             letter->setExpiry( toUint(post(0, 4)) );