Lua Optimize small code function

[Evil Puncker]

Is there any way to optimize this scan function?

local function scanContainer(cid, position)
    local player = Player(cid)
    if not player then
        return
    end
    if not player:isPremium() then
        return
    end
    local corpse = Tile(position):getTopDownItem()
    if not corpse or not corpse:isContainer() then
        return
    end
    if corpse:getType():isCorpse() and corpse:getAttribute(ITEM_ATTRIBUTE_CORPSEOWNER) == cid then
        for a = corpse:getSize() - 1, 0, -1 do
            local containerItem = corpse:getItem(a)
            if containerItem then
                if player then
                    if isItemStackable(containerItem:getId()) then
                        if containerItem:getId() == 2148 then
                            containerItem:remove()
                            player:setBankBalance(player:getBankBalance() + (containerItem:getCount()))
                        end
                        if containerItem:getId() == 2152 then
                            containerItem:remove()
                            player:setBankBalance(player:getBankBalance() + (containerItem:getCount() * 100))
                        end
                        if containerItem:getId() == 2160 then
                            containerItem:remove()
                            player:setBankBalance(player:getBankBalance() + (containerItem:getCount() * 1000))
                        end
                    end
                end
            end
        end
    local g = player:getBankBalance()
    player:sendTextMessage(MESSAGE_EVENT_DEFAULT, "Bank balance: "..g..".")
    end
end

Using tfs 1.3 if it even matters

 

[Infernum]

local multipliers = {
    [2148] = 1,
    [2152] = 100
    [2160] = 1000
}

local function scanContainer(cid, position)
    local player = Player(cid)
    if not player or player and not player:isPremium() then
        return
    end
    local corpse = Tile(position):getTopDownItem()
    if not corpse or not corpse:isContainer() then
        return
    end
    if corpse:getType():isCorpse() and corpse:getAttribute(ITEM_ATTRIBUTE_CORPSEOWNER) == cid then
        for index = corpse:getSize() - 1, 0, -1 do
            local containerItem = corpse:getItem(index)
            if containerItem then
                local itemId = containerItem:getId()
                if isItemStackable(itemId) then
                    local multiplier = multipliers[itemId]
                    if multiplier then
                        containerItem:remove()
                        player:setBankBalance(player:getBankBalance() + (containerItem:getCount() * multiplier))
                    end
                end
            end
        end
        player:sendTextMessage(MESSAGE_EVENT_DEFAULT, "Bank balance: ".. player:getBankBalance() ..".")
    end
end

Not sure why you're using a custom function to check for stackables, you can just use containerItem:getType():isStackable().

Most of what I did was shortening it by using a table and accessing that to multiply from instead of using a variable amount of if statements to do the exact same code for separate IDs (this concept can be useful in many other cases such as this, where you have the same code but with 1 variable dependent upon another).
I also changed the loop variable name (a provides no information, index does because it will contain the current index of the container), I also removed the temporary variable you used to send the bank balance message because it's not needed (it was also mis-tabbed)

 

[leleco95]

why do you need to check if it is stackable?

 

[Infernum]

why do you need to check if it is stackable?

Agreed, no need to check if it's stackable if you're specifically checking for an array of items (which you'd already know if they were stackable or not, since you already know what you're checking for), but I retained his code regardless of that just in case.

 

[leleco95]

Agreed, no need to check if it's stackable if you're specifically checking for an array of items (which you'd already know if they were stackable or not, since you already know what you're checking for), but I retained his code regardless of that just in case.

yeah I understand, my question was directed at OP

also, would it make any difference to do it like this?

local amount = 0
for index = corpse:getSize() - 1, 0, -1 do
    local containerItem = corpse:getItem(index)
    if containerItem then
        local itemId = containerItem:getId()
        if isItemStackable(itemId) then
            local multiplier = multipliers[itemId]
            if multiplier then
                containerItem:remove()
                amount = amount + containerItem:getCount() * multiplier
            end
        end
    end
end
local newBalance = player:getBalance() + amount
player:setBalance(newBalance)
player:sendTextMessage(MESSAGE_EVENT_DEFAULT, "Bank balance: ".. newBalance ..".")

less calls to C++ functions getBalance and setBalance, but idk if it makes any real difference

 

[Infernum]

Accumulation would be faster in theory, but it wouldn't make any difference since the difference in speed i'm talking about is nanoseconds since those functions aren't heavy.

 

[fabian766]

Most of what I did was shortening it by using a table and accessing that to multiply from instead of using a variable amount of if statements to do the exact same code for separate IDs (this concept can be useful in many other cases such as this, where you have the same code but with 1 variable dependent upon another).

I thought he wanted an optimization to the code and using a table isn't going to be faster than several if's because lua use a hash table which are only faster for massive amount of if's.
I wondering if there is any reason for reverse iterating container? It only increase cache misses in this case.

 

[leleco95]

I wondering if there is any reason for reverse iterating container? It only increase cache misses in this case.

he's iterating on size, not capacity
I guess it's because the items are removed, so if you were iterating 0 to last you'd skip some because you're removing them during the walk, as well as the last one's index wouldn't be the same from when you started

 

[Infernum]

I thought he wanted an optimization to the code and using a table isn't going to be faster than several if's because lua use a hash table which are only faster for massive amount of if's.
I wondering if there is any reason for reverse iterating container? It only increase cache misses in this case.

He didn't specify if he wanted better performance or optimizing for flexibility and conciseness, I chose the latter cause it's what I would've done since performance issues are next to nothing with the original code.

 

[Evil Puncker]

sorry guys for the lack of information the code was taken somewhere in the forum and I thought it could be made less "uglier", the main intention was to take coins from monsters and deposit it to bank