• There is NO official Otland's Discord server and NO official Otland's server list. The Otland's Staff does not manage any Discord server or server list. Moderators or administrator of any Discord server or server lists have NO connection to the Otland's Staff. Do not get scammed!

Lua player:addItemEx(item) best practice

C

CastorFlynn

Member
Joined
Aug 29, 2021
Messages
100
Reaction score
15
Comparing the two codes, which would be the most logical and safe?


LUA: 
local shovel = TalkAction("!shovel")
local cost = 100 

function shovel.onSay(player, words, param)
    local item = Game.createItem(2554, 1)
    if not item then
        player:sendCancelMessage("An error occurred while creating the item.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end

    if player:addItemEx(item) ~= RETURNVALUE_NOERROR then
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        player:sendCancelMessage("You do not have enough capacity or space in your inventory.")
        return
    end

    if not player:removeMoneyNpc(cost) then
        player:sendCancelMessage("You do not have enough money.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        player:removeItem(item:getId(), 1) 
        return
    end

    player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You have bought a shovel!")
end

shovel:register()



LUA: 
local shovel = TalkAction("!shovel")

function shovel.onSay(player, words, param)   
    local tempItem = Game.createItem(2554, 1)
    if not tempItem then
        return false
    end

    if player:addItemEx(tempItem) ~= RETURNVALUE_NOERROR then
        tempItem:remove()
        player:sendCancelMessage("You do not have enough capacity to carry this item.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end
    tempItem:remove()

    if not player:removeMoneyNpc(100) then
        player:sendCancelMessage("You do not have enough money.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end
    
    player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
    player:addItem(2554, 1)
    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You have bought a shovel!")
end

shovel:register()
 
Sort by date Sort by votes
There is zero point in the extra work in the second script. You make a "temp" item, check if it gets added to the player, if it doesn't you send message, remove item, and return false... cool... but then you go ahead and remove the item when it is able to be added successfully, and then add it again after the money has been removed (which might actually cause a slot to not be available now)... this is super backwards to be even thinking about approaching your problem this way)... so if I had to pick one, the first example is best simply because it's "safer" and doing less work... however there really isn't anything "unsafe" about either one of these scripts (unless you consider the item going to the ground because a crystal coin was converted to plats or something after they make payment, to be "unsafe" for the player).
Post automatically merged:

I just noticed that your remove in the first one if the npc fails to collect payment... I suppose this is a bit "unsafe" actually... if at all something happened like a forced crash at precisely the wrong moment (we are talking microseconds on timing) the could have both the item and the money... that is very unlikely but if you wanted to practice safe programming patterns, it would be better to have two booleans checked in the beginning that said if player has money for the item, and if play has slot + capacity free for the item, if either of those booleans are false you return false... then from there the remainder of your scope is now safe, its a type of early return strategy along with just deciding the correct order.

I won't rewrite your script for you, but if you could create two new functions if they don't already exist "player:hasMoneyRequired(item)" and "player:canAcceptItem(item, slot) -- this one checks capacity and free slot", and have them both just return true or false, then you can start your talkaction script off with a boolean defined by those function's return values, and handle your early returns like that.. Just a suggestion for how to look at the code
 
Last edited:
Upvote 0 Downvote
The second one is safer since you check capacity + payment before adding an item but it could be little bit better


LUA: 
local shovel = TalkAction("!shovel")
local cost = 100

function shovel.onSay(player, words, param)

    local tempItem = Game.createItem(2554, 1)
    if not tempItem then
        player:sendCancelMessage("An error occurred while creating the item.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end

    local result = player:addItemEx(tempItem)
    if result ~= RETURNVALUE_NOERROR then
        tempItem:remove()
        player:sendCancelMessage("You do not have enough capacity or space in your inventory.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end
   
    tempItem:remove()
 
    if not player:removeMoneyNpc(cost) then
        player:sendCancelMessage("You do not have enough money.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end
   
    player:addItem(2554, 1)

    player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You have bought a shovel!")
end

shovel:register()
 
Upvote 0 Downvote
Sorky said:
The second one is safer since you check capacity + payment before adding an item but it could be little bit better


LUA: 
local shovel = TalkAction("!shovel")
local cost = 100

function shovel.onSay(player, words, param)

    local tempItem = Game.createItem(2554, 1)
    if not tempItem then
        player:sendCancelMessage("An error occurred while creating the item.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end

    local result = player:addItemEx(tempItem)
    if result ~= RETURNVALUE_NOERROR then
        tempItem:remove()
        player:sendCancelMessage("You do not have enough capacity or space in your inventory.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end
  
    tempItem:remove()
 
    if not player:removeMoneyNpc(cost) then
        player:sendCancelMessage("You do not have enough money.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return
    end
  
    player:addItem(2554, 1)

    player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
    player:sendTextMessage(MESSAGE_EVENT_ADVANCE, "You have bought a shovel!")
end

shovel:register()
Click to expand...
I still say you should check if player has the money before bothering with making the item and adding it to the player... but in the end, this version or any of his, they will all work the same, and none are "unsafe"
 
Upvote 0 Downvote
I made this script a long time ago. It is very simple and secure. I added a cooldown, you know why? Players abused it too much in the game, causing the server to slow down and even crash. So, this script is much better and safer, while still being simple.

  • Note: The function player:removeMoney is correct. If you are not sure whether it exists in your TFS, you can use player:removeMoneyNpc and test it. Enjoy!
LUA: 
local TOOL_SHOVEL = {
    id = 2554,
    name = "Shovel",
    cost = 100
}

local exhaust = {}
local exhaustTime = 15

local function buyTool(player, tool)
    local playerId = player:getId()
    local currentTime = os.time()

    if exhaust[playerId] and exhaust[playerId] > currentTime then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "The " .. tool.name .. " is still on cooldown. (" .. (exhaust[playerId] - currentTime) .. "s).")
        return false
    end

    local itemType = ItemType(tool.id)
    local itemWeight = itemType:getWeight()
    local playerCap = player:getFreeCapacity()

    if playerCap < itemWeight then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "You have found a " .. itemType:getName() .. " weighing " .. (itemWeight / 100) .. " oz. It's too heavy.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    local backpack = player:getSlotItem(CONST_SLOT_BACKPACK)
    if not backpack or backpack:getEmptySlots(false) < 1 then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "Your main backpack is full. Free up 1 slot to receive " .. itemType:getName() .. ".")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    if player:removeMoney(tool.cost) then
        player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
        player:say("You bought a " .. tool.name .. ".", TALKTYPE_MONSTER_SAY)
        player:addItem(tool.id, 1)
        exhaust[playerId] = currentTime + exhaustTime
    else
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "You don't have enough gold coins. Cost: [" .. tool.cost .. " gold coins].")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
    end

    return false
end

local Shovel = TalkAction("!shovel", "!pala")
function Shovel.onSay(player, words, param)
    return buyTool(player, TOOL_SHOVEL)
end
Shovel:register()
 
Upvote 0 Downvote
Mateus Robeerto said:
I made this script a long time ago. It is very simple and secure. I added a cooldown, you know why? Players abused it too much in the game, causing the server to slow down and even crash. So, this script is much better and safer, while still being simple.

  • Note: The function player:removeMoney is correct. If you are not sure whether it exists in your TFS, you can use player:removeMoneyNpc and test it. Enjoy!
LUA: 
local TOOL_SHOVEL = {
    id = 2554,
    name = "Shovel",
    cost = 100
}

local exhaust = {}
local exhaustTime = 15

local function buyTool(player, tool)
    local playerId = player:getId()
    local currentTime = os.time()

    if exhaust[playerId] and exhaust[playerId] > currentTime then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "The " .. tool.name .. " is still on cooldown. (" .. (exhaust[playerId] - currentTime) .. "s).")
        return false
    end

    local itemType = ItemType(tool.id)
    local itemWeight = itemType:getWeight()
    local playerCap = player:getFreeCapacity()

    if playerCap < itemWeight then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "You have found a " .. itemType:getName() .. " weighing " .. (itemWeight / 100) .. " oz. It's too heavy.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    local backpack = player:getSlotItem(CONST_SLOT_BACKPACK)
    if not backpack or backpack:getEmptySlots(false) < 1 then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "Your main backpack is full. Free up 1 slot to receive " .. itemType:getName() .. ".")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    if player:removeMoney(tool.cost) then
        player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
        player:say("You bought a " .. tool.name .. ".", TALKTYPE_MONSTER_SAY)
        player:addItem(tool.id, 1)
        exhaust[playerId] = currentTime + exhaustTime
    else
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED,
            "You don't have enough gold coins. Cost: [" .. tool.cost .. " gold coins].")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
    end

    return false
end

local Shovel = TalkAction("!shovel", "!pala")
function Shovel.onSay(player, words, param)
    return buyTool(player, TOOL_SHOVEL)
end
Shovel:register()
Click to expand...

Im kinda curious why u create a local function for that?
You can put all that into the main function
 
Upvote 0 Downvote
Marko999x said:
Im kinda curious why u create a local function for that?
You can put all that into the main function
Click to expand...
With buy item logic extracted to separate function, he can easily add more talkactions for other items.
He can add at end of this file:
LUA: 
local TOOL_ROPE = {
    id = 123,
    name = "Rope",
    cost = 100
}
local Rope= TalkAction("!rope")
function Rope.onSay(player, words, param)
    return buyTool(player, TOOL_ROPE)
end
Rope:register()
and now this script sells shovel and rope.
 
Upvote 0 Downvote
Marko999x said:
Im kinda curious why u create a local function for that?
You can put all that into the main function
Click to expand...
There is another method here. The script is complete and authentic. Here, you can understand how it works... in a simple and easy way to modify. You can add multiple talkactions whenever you want.

LUA: 
local exhaust = {}
local exhaustTime = 30

local function buyTool(player, toolName, itemId, itemCost)
    local playerId = player:getId()
    local currentTime = os.time()

    if exhaust[playerId] and exhaust[playerId] > currentTime then
        player:sendCancelMessage("The " .. toolName .. " is still on cooldown. (" .. (exhaust[playerId] - currentTime) .. "s).")
        return false
    end

    local itemType = ItemType(itemId)
    local itemWeight = itemType:getWeight()
    local playerCap = player:getFreeCapacity()

    if playerCap < itemWeight then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED, "You have found a " .. itemType:getName() .. " weighing " .. (itemWeight / 100) .. " oz. It's too heavy.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    local backpack = player:getSlotItem(CONST_SLOT_BACKPACK)
    if not backpack or backpack:getEmptySlots(false) < 1 then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED, "Your main backpack is full. You need to free up 1 available slot to get " .. itemType:getName() .. ".")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end

    if player:removeMoney(itemCost) then
        player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
        player:say("You bought a " .. toolName .. ".")
        player:addItem(itemId, 1)
        exhaust[playerId] = currentTime + exhaustTime
    else
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        player:sendCancelMessage("You don't have enough gold coins. Cost: [" .. itemCost .. " gold coins].")
    end

    return false
end

local Machete = TalkAction("!machete", "!machet")
function Machete.onSay(player, words, param)
    return buyTool(player, "Machete", 3308, 1000)
end
Machete:register()

local Pick = TalkAction("!pick", "!pickaxe", "!pico")
function Pick.onSay(player, words, param)
    return buyTool(player, "Pick Axe", 3456, 1000)
end
Pick:register()

local Shovel = TalkAction("!shovel", "!pala")
function Shovel.onSay(player, words, param)
    return buyTool(player, "Shovel", 3457, 1000)
end
Shovel:register()

local Rope = TalkAction("!rope", "!cuerda")
function Rope.onSay(player, words, param)
    return buyTool(player, "Rope", 3003, 1000)
end
Rope:register()

local Tools = TalkAction("!tools")
function Tools.onSay(player, words, param)
    player:sendTextMessage(MESSAGE_STATUS_CONSOLE_BLUE, "You can use the command, !shovel, !machete, !pickaxe and !rope.")
    player:sendCancelMessage("You can use the command, !shovel, !machete, !pickaxe and !rope.")
    return false
end
Tools:register()
 
Upvote 0 Downvote
LUA: 
local exhaust = {}
local exhaustTime = 5

local config = {
    ['!machete'] = {itemId = 2420, itemAmount = 1, itemCost = 100},
    ['!rope'] = {itemId = 2120, itemAmount = 1, itemCost = 100},
}

local talkaction = TalkAction("!rope", "!machete")

function talkaction.onSay(player, words, param)
    local playerId = player:getId()
    local currentTime = os.time()
    local itemWord = config[words]
    if not words then
        return false
    end
    if exhaust[playerId] and exhaust[playerId] > currentTime then
        player:sendCancelMessage("You are exhausted.")
        return false
    end
    local itemType = ItemType(itemWord.itemId)
    local itemWeight = itemType:getWeight()
    local playerCap = player:getFreeCapacity()
    if playerCap < itemWeight then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED, "You have found a " .. itemType:getName() .. " weighing " .. (itemWeight / 100) .. " oz. It's too heavy.")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end
    local backpack = player:getSlotItem(CONST_SLOT_BACKPACK)
    if not backpack or backpack:getEmptySlots(false) < 1 then
        player:sendTextMessage(MESSAGE_STATUS_CONSOLE_RED, "Your main backpack is full. You need to free up 1 available slot to get " .. itemType:getName() .. ".")
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        return false
    end
    if player:removeMoney(itemWord.cost) then
        local boughtItem = player:addItem(itemWord.itemId, itemWord.itemAmount)
        player:getPosition():sendMagicEffect(CONST_ME_MAGIC_RED)
        player:say("You bought a " .. ItemType(boughtItem:getId()):getName() .. ".")
        exhaust[playerId] = currentTime + exhaustTime
    else
        player:getPosition():sendMagicEffect(CONST_ME_POFF)
        player:sendCancelMessage("You don't have enough gold coins. Cost: [" .. itemId.itemCost .. " gold coins].")
    end
    return false
end

talkaction:register()

@Gesior.pl @Mateus Robeerto
Here this is a example how I would do it and what I meant by put it in the main function :D
At the end of the day both does their job xD
 
Upvote 0 Downvote
Marko999x said:
Here this is a example how I would do it and what I meant by put it in the main function
Click to expand...
You can replace:
LUA: 
local config = {
    ['!machete'] = {itemId = 2420, itemAmount = 1, itemCost = 100},
    ['!rope'] = {itemId = 2120, itemAmount = 1, itemCost = 100},
}

local talkaction = TalkAction("!rope", "!machete")
with:
LUA: 
local config = {
    ['!machete'] = {itemId = 2420, itemAmount = 1, itemCost = 100},
    ['!rope'] = {itemId = 2120, itemAmount = 1, itemCost = 100},
}

local wordsList = {}
for word, config in pairs(config) do
    table.insert(wordsList, word)
end

local talkaction = TalkAction(table.unpack(wordsList))
to make it use strings from config.

EDIT:
On topic:
player:addItemEx(tempItem) is better than player:addItem(itemId), because it returns nil, false or ReturnValue.
addItem may return: nil, false, Item or table of `Item`s. nil/false are for any kind of error.
addItemEx may return: nil, false or ReturnValue. nil/false are only for serious errors like not existing player/item, all other problems are reported as ReturnValue. If item was added without error, it returns RETURNVALUE_NOERROR. If there was some problem, it returns one of many possible errors ex. RETURNVALUE_NOTENOUGHCAPACITY for no cap, RETURNVALUE_NOTENOUGHROOM for no free place in BP.
 
Last edited:
Upvote 0 Downvote
Gesior.pl said:
EDIT:
On topic:
player:addItemEx(tempItem) is better than player:addItem(itemId), because it returns nil, false or ReturnValue.
addItem may return: nil, false, Item or table of `Item`s. nil/false are for any kind of error.
addItemEx may return: nil, false or ReturnValue. nil/false are only for serious errors like not existing player/item, all other problems are reported as ReturnValue. If item was added without error, it returns RETURNVALUE_NOERROR. If there was some problem, it returns one of many possible errors ex. RETURNVALUE_NOTENOUGHCAPACITY for no cap, RETURNVALUE_NOTENOUGHROOM for no free place in BP.
Click to expand...
imagine such info was available in scripting reference.
 
Upvote 0 Downvote

Similar threads

G
  • Question Question
TFS 1.X+ Conjure 2 runes in hand
Replies
3
Views
163
gordaum
G
Back
Top