[Game] Code Audit

[Red]

Hello!

I wanted to try to start a game that I haven't seen on OTLand before. The premise is simple: you present a piece of code to the community that is erroneous in one or more ways and the community will reply to your code and point out where the errors are and why.

A few guidelines to posting:

1. Tell us the issue with the code. For example:

• Does the code fail to run?
• Does the code produce unwanted behavior?
• Does the code have a syntax error?

2. Provide background information necessary to properly audit the code. For example:

• Lua script, TFS 1.x, the following functions are custom functions and work properly (list)

3. Provide any ancillary information you might want us to know.

Hopefully the idea isn't too confusing and ends up being fun. You're not limited to Lua nor TFS functions; as long as enough information is provided, it can be anything!


THIS IS NOT A "PLEASE FIX MY CODE" THREAD. THE USERS POSTING CODE TO BE AUDITED SHOULD KNOW THE ANSWER TO WHAT'S WRONG WITH THEIR CODE PRIOR TO POSTING.
Users that want their code audited and don't know what's wrong with it should get it audited here: https://otland.net/forums/support.16/

Red

 

[Red]

I'll start with an example!

1. The code produces unwanted behavior.
2. Lua, TFS 1.2.
item:setCustomAttribute(ATTR_CUSTOM_CRITICAL_CHANCE, 5) is a custom function. There is nothing wrong in this line.
3. N/A

function onUse(player, item, fromPosition, target, toPosition, isHotkey)
   if player:getStorageValue(item.uid) == 1 then
       player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'The chest is empty.')
       return true
   end

   local item = player:addItem(2515, 1)
   if not item then
       return true
   end

   item:setCustomAttribute(ATTR_CUSTOM_CRITICAL_CHANCE, 5)

   if player:getLevel() < 350 then
       player:addExperience(5000, true)
   end

   player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'You have found a guardian shield [+5% Critical Chance].')
   player:setStorageValue(item.uid, 1)
   return true
end

Red

 

[Peonso]

Should use addItemEx, don't you? Else you won't be able to set the attribute.

 

[Printer]

No idea, try this:

function onUse(player, item, fromPosition, target, toPosition, isHotkey)
    if player:getStorageValue(item.uid) == 1 then
       player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'The chest is empty.')
       return true
    end

    local item = Game.createItem(2515, 1)
    if not item then
        return true
    end

    item:setCustomAttribute(ATTR_CUSTOM_CRITICAL_CHANCE, 5)
    if player:addItemEx(item) ~= RETURNVALUE_NOERROR then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_BLUE, "Not enough of room or cap.")
        return true
    end

    if player:getLevel() < 350 then
       player:addExperience(5000, true)
    end

    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'You have found a guardian shield [+5% Critical Chance].')
    player:setStorageValue(item.uid, 1)
    return true
end

 

[Ninja]

Unwanted behavior

Adds an infinite number of guardian shields.
​

Reason

The initial Item object reference is overwritten when the local variable 'item' is declared. The unique id (item.uid) at the beginning is not equal to the one you find at the end.
​

Solution

Don't overwrite the references passed to the callback. Use a unique variable name instead (e.g. shieldItem).
​

Next one, please!

 

[Red]

Should use addItemEx, don't you? Else you won't be able to set the attribute.

The attribute is able to be set without addItemEx.

No idea, try this:

function onUse(player, item, fromPosition, target, toPosition, isHotkey)
    if player:getStorageValue(item.uid) == 1 then
       player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'The chest is empty.')
       return true
    end

    local item = Game.createItem(2515, 1)
    if not item then
        return true
    end

    item:setCustomAttribute(ATTR_CUSTOM_CRITICAL_CHANCE, 5)
    if player:addItemEx(item) ~= RETURNVALUE_NOERROR then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_BLUE, "Not enough of room or cap.")
        return true
    end

    if player:getLevel() < 350 then
       player:addExperience(5000, true)
    end

    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, 'You have found a guardian shield [+5% Critical Chance].')
    player:setStorageValue(item.uid, 1)
    return true
end

The code you've provided works the same as the code I've provided but in a different way. Unfortunately, the unwanted behavior persists.

Unwanted behavior

Adds an infinite number of guardian shields.
​

Reason

The initial Item object reference is overwritten when the local variable 'item' is declared. The unique id (item.uid) at the beginning is not equal to the one you find at the end.
​

Solution

Don't overwrite the references passed to the callback. Use a unique variable name instead (e.g. shieldItem).
​

Next one, please!

Ding ding!

Unfortunately, this is the only "test" I have for now. I'll try to think of more later. Hopefully some others think up some as well!

Red

 

[whitevo]

I like the thread idea, but it was harder to think of something when you actually start thinking of it. I will create a "code puzzle" some other time.
This is pretty simple so Ninja and Red should definitely not answer this xD

1. Tell us the issue with the code.

• current code should crash the server (i think)

2. Provide background information necessary to properly audit the code.

• setText() method is working as intended.

3. Provide any ancillary information you might want us to know.

ancillary sure is a fancy word.

​

function brewing_createUnfinishedPotion(player, item, itemEx)
    if itemEx:getId() ~= 2006 then return end
    if itemEx:getFluidType() ~= 1 then return player:sendTextMessage(GREEN, "currently potions can only be created with vial of water") end
local unfinishedPotion = player:addItem(10031, 1)

    item:remove(1)
    itemEx:remove(1)
    unfinishedPotion:setText("powderAID", item:getActionId())
    return doSendMagicEffect(player:getPosition(), 28)
end

 

[imkingran]

@Red
I like this thread idea, I think it will be a fun way to learn new things!

function brewing_createUnfinishedPotion(player, item, itemEx)
    if itemEx:getId() ~= 2006 then return end
    if itemEx:getFluidType() ~= 1 then return player:sendTextMessage(GREEN, "currently potions can only be created with vial of water") end
local unfinishedPotion = player:addItem(10031, 1)

    item:remove(1)
    itemEx:remove(1)
    unfinishedPotion:setText("powderAID", item:getActionId())
    return doSendMagicEffect(player:getPosition(), 28)
end

Unwanted Behavior: (tested with TFS 1.1)

Initially I thought that calling item:getActionId() after the item was already deleted would be the problem but I actually just tested it and seems to work fine. So I guess I'm not sure what the error is but here are some changes I made:
(instead of using setText, I tested with: unfinishedPotion:setActionId(item:getActionId()))

Spoiler

function brewing_createUnfinishedPotion(player, item, itemEx)
    if itemEx:getId() ~= 2006 then return end
    if itemEx:getFluidType() ~= 1 then return player:sendTextMessage(MESSAGE_INFO_DESCR, "Currently potions can only be created with vial of water.") end
    local unfinishedPotion = player:addItem(10031, 1, false) -- addItem(itemID, count, CanDropOnMap = true/false)
    if not unfinishedPotion then -- since it CAN'T drop on the map then if it doesn't exist the player didn't have enough space or cap to hold it.
        return player:sendCancelMessage('Error: You did not receive the potion, process terminated! [Not enough capacity/space!]') -- If the potion wasn't delivered then we stop before we remove their items
    end
    unfinishedPotion:setActionId(item:getActionId())
    item:remove(1)
    itemEx:remove(1)
    return player:getPosition():sendMagicEffect(CONST_ME_GIFT_WRAPS)
end

function onUse(player, item, fromPosition, itemEx, toPosition)
    return brewing_createUnfinishedPotion(player, item, itemEx)
end

Spoiler: Trying to see if calling item:getActionId after it's removed will cause an error

function brewing_createUnfinishedPotion(player, item, itemEx)
    if itemEx:getId() ~= 2006 then return end
    if itemEx:getFluidType() ~= 1 then return player:sendTextMessage(MESSAGE_INFO_DESCR, "currently potions can only be created with vial of water") end
    local unfinishedPotion = player:addItem(10031, 1, false) -- addItem(itemID, count, CanDropOnMap = true/false)
    if not unfinishedPotion then -- since it CAN'T drop on the map then if it doesn't exist the player didn't have enough space or cap to hold it.
        return player:sendCancelMessage('Error: You did not receive the potion, process terminated! [Not enough capacity/space!]')
    end
        item:remove(1)
        itemEx:remove(1)
    print(item:getName())
    print(item:getActionId())
    unfinishedPotion:setActionId(item:getActionId())
    return player:getPosition():sendMagicEffect(CONST_ME_GIFT_WRAPS)
end

function onUse(player, item, fromPosition, itemEx, toPosition)
    return brewing_createUnfinishedPotion(player, item, itemEx)
end

printed values:

mace
3000 -- (I used a mace and set it's action id to 3000)

Other possible errors:

Spoiler

I'm not sure if you were considering these as errors or not:

doSendMagicEffect(player:getPosition(), 28)

- 28 should be replaced with the CONST_ME_XXXX, I thnk it's more proper? but it's w/e
- The rest of the code was in TFS 1.X format but this line used a compat function --> player:getPosition():sendMagicEffect(CONST_ME_GIFT_WRAPS)

player:sendTextMessage(GREEN, "currently potions can only be created with vial of water")

I believe in the official repository to send the green text we should use the constant name below:
GREEN --> MESSAGE_INFO_DESCR

​

Look forward to hearing what I missed and what the correct answer actually is!

​

 

[whitevo]

well your first though was correct.
should not use methods on userdata what is removed from game.

Not sure why It didn't crash though, usually doing something like this is insta death.
Nice found on the GREEN enum, didn't even think about that xD

about your secondary solution. Good job. You turned the unfinished potion into herb powder by setting the powder actionID to it

 

[whitevo]

Be a man and solve this problem by looking the code! (it will be too easy if you let your program check where it went wrong)

1. Tell us the issue with the code.

• print value is not intended

2. Provide background information necessary to properly audit the code.

• TFS 1.2

3. Provide any ancillary information you might want us to know.

• tableCount(t) -- RETURNS number of keys in table

local loopConf = {
    [{1, "this", "is"}] = {"puzzle", "and"},
    [{2, "was", "super"}] = {"hard", "core"},
    [{3, "to", "randomly"}] = {"make", "tables"},
}

function this_loop_is_real()
local textT = {}
local loopID = 0
local sequenceID = 0
local finalText = ""

    local function insertValue(text)
        if tonumber(text) then return end
        loopID = loopID + 1
        textT[loopID] = text
    end
   
    local function thisFunctionNameRuinedTheFunSoIChangedIt(textT)
        local delete = false
       
        local function deleteText(key)
            if not delete then delete = true return end
            textT[key] = nil
            delete = false
        end
       
        for key, text in ipairs(textT) do deleteText(key) end
    end
   
    for x=1, tableCount(loopConf) do
        sequenceID = sequenceID + 1
        for keyT, valueT in pairs(loopConf) do
            if keyT[1] == sequenceID then
                for i, v in ipairs(keyT) do insertValue(v) end
                for i, v in ipairs(valueT) do insertValue(v) end
                break
            end
        end
    end
   
    thisFunctionNameRuinedTheFunSoIChangedIt(textT)
    for _, word in ipairs(textT) do finalText = finalText.." "..word end
    print(finalText)
end