From 5e602c15fc265dae25e2524298299bbb482715d5 Mon Sep 17 00:00:00 2001 From: Eduardo Dantas Date: Tue, 29 Oct 2024 18:40:32 -0300 Subject: [PATCH 1/2] refactor: Container::create for readability and variable shadow fix This commit refactors the `Container::create` function to enhance code readability and maintainability. The changes include the introduction of the `continue` statement to reduce nesting and clarify the conditions under which items are added to the container. This approach avoids deep nesting and makes the conditions more straightforward. Key changes: - Introduced `isItemValid` variable to consolidate the item validation logic into a single, readable condition. - Used `continue` to skip iterations early if an item does not meet the required conditions, thereby flattening the loop structure. - Fixed a critical issue where two different `container` variables were used confusingly within the same loop, potentially leading to bugs. Now, `newContainer` is used consistently to refer to the container being populated, and `item->getContainer()` checks are correctly handled without variable shadowing. These changes ensure the function is cleaner and less prone to errors, making the logic behind item addition more transparent. --- src/game/game.cpp | 4 +-- src/items/containers/container.cpp | 34 ++++++++++++++------ src/items/containers/container.hpp | 16 ++++++++- src/server/network/protocol/protocolgame.cpp | 2 +- 4 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/game/game.cpp b/src/game/game.cpp index c6ba09311d5..540558e2121 100644 --- a/src/game/game.cpp +++ b/src/game/game.cpp @@ -4072,7 +4072,7 @@ void Game::playerMoveUpContainer(uint32_t playerId, uint8_t cid) { auto it = browseFields.find(tile); if (it == browseFields.end() || it->second.expired()) { - parentContainer = Container::create(tile); + parentContainer = Container::createBrowseField(tile); browseFields[tile] = parentContainer; } else { parentContainer = it->second.lock(); @@ -4626,7 +4626,7 @@ void Game::playerBrowseField(uint32_t playerId, const Position &pos) { auto it = browseFields.find(tile); if (it == browseFields.end() || it->second.expired()) { - container = Container::create(tile); + container = Container::createBrowseField(tile); browseFields[tile] = container; } else { container = it->second.lock(); diff --git a/src/items/containers/container.cpp b/src/items/containers/container.cpp index 595ae38dedf..f4f8ccdf4b3 100644 --- a/src/items/containers/container.cpp +++ b/src/items/containers/container.cpp @@ -43,20 +43,36 @@ std::shared_ptr Container::create(uint16_t type, uint16_t size, bool return std::make_shared(type, size, unlocked, pagination); } -std::shared_ptr Container::create(std::shared_ptr tile) { - auto container = std::make_shared(ITEM_BROWSEFIELD, 30, false, true); - TileItemVector* itemVector = tile->getItemList(); +std::shared_ptr Container::createBrowseField(const std::shared_ptr &tile) { + const auto &newContainer = create(ITEM_BROWSEFIELD, 30, false, true); + if (!newContainer || !tile) { + return nullptr; + } + + const TileItemVector* itemVector = tile->getItemList(); if (itemVector) { - for (auto &item : *itemVector) { - if (((item->getContainer() || item->hasProperty(CONST_PROP_MOVABLE)) || (item->isWrapable() && !item->hasProperty(CONST_PROP_MOVABLE) && !item->hasProperty(CONST_PROP_BLOCKPATH))) && !item->hasAttribute(ItemAttribute_t::UNIQUEID)) { - container->itemlist.push_front(item); - item->setParent(container); + for (const auto &item : *itemVector) { + if (!item) { + continue; } + + // Checks if the item has an internal container, is movable, or is packable without blocking the path. + bool isItemValid = item->getContainer() || item->hasProperty(CONST_PROP_MOVABLE) || (item->isWrapable() && !item->hasProperty(CONST_PROP_MOVABLE) && !item->hasProperty(CONST_PROP_BLOCKPATH)); + + // If the item has a unique ID or is not valid, skip to the next item. + if (item->hasAttribute(ItemAttribute_t::UNIQUEID) || !isItemValid) { + continue; + } + + // Add the item to the new container and set its parent. + newContainer->itemlist.push_front(item); + item->setParent(newContainer); } } - container->setParent(tile); - return container; + // Set the parent of the new container to be the tile. + newContainer->setParent(tile); + return newContainer; } Container::~Container() { diff --git a/src/items/containers/container.hpp b/src/items/containers/container.hpp index e07b2edff3a..f37383c4f10 100644 --- a/src/items/containers/container.hpp +++ b/src/items/containers/container.hpp @@ -138,7 +138,21 @@ class Container : public Item, public Cylinder { static std::shared_ptr create(uint16_t type); static std::shared_ptr create(uint16_t type, uint16_t size, bool unlocked = true, bool pagination = false); - static std::shared_ptr create(std::shared_ptr type); + + /** + * @brief Creates a container for browse field functionality with items from a specified tile. + * + * This function generates a new container specifically for browse field use, + * populating it with items that meet certain criteria from the provided tile. Items + * that can be included must either have an internal container, be movable, or be + * wrapable without blocking path and without a unique ID. + * + * @param tile A shared pointer to the Tile from which items will be sourced. + * @return std::shared_ptr Returns a shared pointer to the newly created Container if successful; otherwise, returns nullptr. + * + * @note This function will return nullptr if the newContainer could not be created or if the tile pointer is null. + */ + static std::shared_ptr createBrowseField(const std::shared_ptr &type); // non-copyable Container(const Container &) = delete; diff --git a/src/server/network/protocol/protocolgame.cpp b/src/server/network/protocol/protocolgame.cpp index ae67bad447f..67adacf2399 100644 --- a/src/server/network/protocol/protocolgame.cpp +++ b/src/server/network/protocol/protocolgame.cpp @@ -4607,7 +4607,7 @@ void ProtocolGame::sendUnjustifiedPoints(const uint8_t &dayProgress, const uint8 } void ProtocolGame::sendContainer(uint8_t cid, std::shared_ptr container, bool hasParent, uint16_t firstIndex) { - if (!player) { + if (!player || !container) { return; } From 3e541242986103496ee54260def8488f55428738 Mon Sep 17 00:00:00 2001 From: GitHub Actions Date: Thu, 31 Oct 2024 17:58:01 +0000 Subject: [PATCH 2/2] Code format - (Clang-format) --- src/creatures/monsters/spawns/spawn_monster.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/creatures/monsters/spawns/spawn_monster.cpp b/src/creatures/monsters/spawns/spawn_monster.cpp index 153db66dee4..7d7bbc84e34 100644 --- a/src/creatures/monsters/spawns/spawn_monster.cpp +++ b/src/creatures/monsters/spawns/spawn_monster.cpp @@ -327,7 +327,7 @@ void SpawnMonster::scheduleSpawn(uint32_t spawnMonsterId, spawnBlock_t &sb, cons } void SpawnMonster::cleanup() { - for (auto it = spawnedMonsterMap.begin(); it != spawnedMonsterMap.end(); ) { + for (auto it = spawnedMonsterMap.begin(); it != spawnedMonsterMap.end();) { const auto &monster = it->second; if (!monster || monster->isRemoved()) { auto spawnIt = spawnMonsterMap.find(it->first);