TFS 1.3 Script working but want to make it better

[Marko999x]

Hey
Im kinda new in lua and would like to know how it would be "better" to make this script
The script is working as it should but is it fine like this? or is there a better way?

function onUse(player, item, fromPosition, target, toPosition, isHotkey)

    if player:getLevel() >= 10000 then
    player:sendTextMessage(MESSAGE_INFO_DESCR, "You must be atleast level 10000 to use this item!")
    return true
    end
   
    if player:getVocation() == 1 or 2 or 3 or 4 then
    player:sendTextMessage(MESSAGE_INFO_DESCR, "You must buy first promotion to use this item!")
    return true
    end
   
    if player:getVocation() == 9 or 10 or 11 or 12 then
    player:sendTextMessage(MESSAGE_INFO_DESCR, "You alredy have been promoted to a epic vocation!")
    return true
    end
   
    if player:getVocation() == 5 or 6 or 7 or 8 then
    local voc = player:getVocation():getId()
      --  item:remove(1)
        player:setVocation(voc + 4)
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You have been promoted to a " .. player:getVocation():getName() .. "!")
        player:getPosition():sendMagicEffect(27)
        end
    return true
end

The part I dislike is == 5 or 6 or 7 or 8
is there a way to make it better or easier?

 

[roriscrave]

You cant use this:

 if player:getVocation() == 1 or 2 or 3 or 4 then

U need to use:

 if player:getVocation() == 1 or  player:getVocation() == 2 or  player:getVocation() == 3 or  player:getVocation() == 4 then

but is better do this thing, because u call a function getVocation() only 1 time:

local pVoc = player:getVocation()
if pVoc == 1 or  pVoc == 2 or  pVoc == 3 or  pVoc == 4 then

 

[Marko999x]

Do I have to use "or" in that case?

or is there a other way ?

 

[roriscrave]

Do I have to use "or" in that case?

or is there a other way ?

when there are several OR, OR, OR, it is better to use a table

if table.contains(table, value)

function onUse(player, item, fromPosition, target, toPosition, isHotkey)

    if player:getLevel() >= 10000 then
    player:sendTextMessage(MESSAGE_INFO_DESCR, "You must be atleast level 10000 to use this item!")
        return true
    end
 
    local pVoc = player:getVocation():getId()
    if table.contains({1, 2, 3, 4}, pVoc) then
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You must buy first promotion to use this item!")
    elseif table.contains({9, 10, 11, 12}, pVoc) then
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You alredy have been promoted to a epic vocation!")
    elseif table.contains({5, 6, 7, 8}, pVoc) then
      --  item:remove(1)
        player:setVocation(pVoc + 4)
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You have been promoted to a " .. player:getVocation():getName() .. "!")
        player:getPosition():sendMagicEffect(27)
    end
    return true
end

 

[Xikini]

    if player:getLevel() >= 10000 then

should be <, no?

 

[Marko999x]

should be <, no?

was my fault

 

[Nekiro]

when there are several OR, OR, OR, it is better to use a table

if table.contains(table, value)

function onUse(player, item, fromPosition, target, toPosition, isHotkey)

    if player:getLevel() >= 10000 then
    player:sendTextMessage(MESSAGE_INFO_DESCR, "You must be atleast level 10000 to use this item!")
        return true
    end

    local pVoc = player:getVocation():getId()
    if table.contains({1, 2, 3, 4}, pVoc) then
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You must buy first promotion to use this item!")
    elseif table.contains({9, 10, 11, 12}, pVoc) then
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You alredy have been promoted to a epic vocation!")
    elseif table.contains({5, 6, 7, 8}, pVoc) then
      --  item:remove(1)
        player:setVocation(pVoc + 4)
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You have been promoted to a " .. player:getVocation():getName() .. "!")
        player:getPosition():sendMagicEffect(27)
    end
    return true
end

player:getVocation() returns vocation object not vocation id

 

[roriscrave]

player:getVocation() returns vocation object not vocation id

yeh, i changed it in another (next) post

 

[Alpha]

This is based on the assumption that you will only have 1 further promotion after the first. It will break if you add a 3rd promotion.

local requiredLevel = 10000

function onUse(player, item, fromPosition, target, toPosition, isHotkey)
	if player:getLevel() < requiredLevel then
		player:sendTextMessage(MESSAGE_INFO_DESCR, ("You must be at least level %d to use this item!"):format(requiredLevel))
		return true
	end
   
   	local vocation = player:getVocation()
	if not vocation:getDemotion() then
		player:sendTextMessage(MESSAGE_INFO_DESCR, "You must buy first promotion to use this item!")
		return true
	end

	local promotedVocation = vocation:getPromotion()   
	if not promotedVocation then
		player:sendTextMessage(MESSAGE_INFO_DESCR, ("You have already been promoted to %s!"):format(vocation:getDescription()))
		return true
	end
   
	item:remove()
	player:setVocation(promotedVocation:getId())
	player:sendTextMessage(MESSAGE_INFO_DESCR, ("You have been promoted to %s!"):format(promotedVocation:getDescription()))
	player:getPosition():sendMagicEffect(CONST_ME_CRAPS)
	return true
end

 

[Nekiro]

This is based on the assumption that you will only have 1 further promotion after the first. It will break if you add a 3rd promotion.

local requiredLevel = 10000

function onUse(player, item, fromPosition, target, toPosition, isHotkey)
    if player:getLevel() < requiredLevel then
        player:sendTextMessage(MESSAGE_INFO_DESCR, ("You must be at least level %d to use this item!"):format(requiredLevel))
        return true
    end
 
       local vocation = player:getVocation()
    if not vocation:getDemotion() then
        player:sendTextMessage(MESSAGE_INFO_DESCR, "You must buy first promotion to use this item!")
        return true
    end

    local promotedVocation = vocation:getPromotion() 
    if not promotedVocation then
        player:sendTextMessage(MESSAGE_INFO_DESCR, ("You have already been promoted to %s!"):format(vocation:getDescription()))
        return true
    end
 
    item:remove()
    player:setVocation(promotedVocation:getId())
    player:sendTextMessage(MESSAGE_INFO_DESCR, ("You have been promoted to %s!"):format(promotedVocation:getDescription()))
    player:getPosition():sendMagicEffect(CONST_ME_CRAPS)
    return true
end

You can check promoted vocation in a loop and it will always work no matter how many promotions vocation can have.
getPromotion returns nil if there is no promotion

 

[Alpha]

You can check promoted vocation in a loop and it will always work no matter how many promotions vocation can have.
getPromotion returns nil if there is no promotion

I'm well aware, but from the text he initially had there, it seemed like he wants this item to only work for the "epic" promotion. But the code I made there would also promote you to the promotiong beyond "epic", if there was one after it and the item was used, which is why I mentioned it. Just didn't feel like explicitly locking it down to specific vocation id's and using table.contains, simply checking if the player is already on the last promotion is cleaner.

Guess I shouldn't have said it will break, it won't break. It will just have potentially unintended behaviour.