Is my code correctly written?

Hi!
If it isn’t too much to ask, could someone read through my code and see if I’m “doing it right” or what to call it?
I’m pretty green when it comes to lua, so I don’t always fully understand what I’m doing.

It’s an item for the Pointshop by _Undefined, which increases the player’s walk/run speed for 5 minutes.
I’ve tested it in single player and it works fine, but I’m not 100% sure if there could be problems in multiplayer.


ITEM.Name = 'Speed+'
ITEM.Price = 50
ITEM.Model = 'models/props_junk/GlassBottle01a.mdl'		--temporary icon
ITEM.NoPreview = true
ITEM.SingleUse = true
ITEM.Multiple = true			--A custom item property, which will notify the player they can boost the powerup by purchasing it multiple times.



function ITEM:OnBuy(ply)		--When you buy the item...

	ply.WalkSpeed = math.min(400, ply:GetWalkSpeed() + 50)				--set the WalkSpeed variable slightly higher than current walk speed.
	ply.RunSpeed = math.min(800, ply:GetRunSpeed() + 100)				--same as above, but for run speed.
	local timername = ply:UniqueID() .. "_reset_" .. self.Name			--get unique timer name depending on player ID and item name
	local refreshtimer = ply:UniqueID() .. "_refresh_" .. self.Name		--same as above, but for a second timer.

	
	
--This function resets speed back to normal (called when powerup time runs out)
	local function ResetPowerup()
		if IsValid( ply ) then
			ply:SetWalkSpeed(200)
			ply:SetRunSpeed(400)
			ply:PS_Notify(self.Name .. " has worn off")
		end
		timer.Remove( timername )
		timer.Remove( refreshtimer )
	end

	
	
	--This function uses the WalkSpeed/RunSpeed variables and applies them to the player.
	--the refresh timer will call this every 2 seconds, because if player dies his speed is auto-reset.
	local function RefreshPowerup()
		if IsValid( ply ) then
			ply:SetWalkSpeed(math.max(ply:GetWalkSpeed(), ply.WalkSpeed))
			ply:SetRunSpeed(math.max(ply:GetRunSpeed(), ply.RunSpeed))
		end
	end
	
	
	
	--increase time if timer exists, otherwise create the timer which resets speed back to normal.
	if timer.Exists( timername ) then
		timer.Adjust( timername, timer.TimeLeft(timername) + 180, 1, ResetPowerup)		
	else
		timer.Create( timername , 300, 1, ResetPowerup)
	end
	
	
	
	
	--Creates a timer which will call the refresh function every 2 seconds.
	if not timer.Exists( refreshtimer ) then
		timer.Create( refreshtimer , 2, 0, RefreshPowerup)	
	end

end
--end of the ITEM:OnBuy function



-- only allow the player to purchase the item if his speed is below a certain amount.
function ITEM:CanPlayerBuy(ply)
	return ( ply:GetRunSpeed() < 800 and ply:GetWalkSpeed() < 400 )
end

how about you actually test it in multiplayer (create a listen server) and find out yourself
also why are you running a timer every 2 seconds to reset the players speed?
you should only set it when it actually changes or when the player spawns

It works on a dedicated server as well, I’m more wondering if I’m correctly creating unique variables and timers for each player who buys the item.

I wasn’t sure how to use the PlayerSpawn function properly so it targets the correct player, so that’s why I’m using the 2 second timer.
Normally with pointshop items,you hook the functions like this: “ITEM.PlayerSpawn”, but since the item is single use and not actually equipped by the player, the spawn function wasn’t executed.

Listen server != SRCDS

its pretty close though, 99% of all code ive tested on a listen server also worked in srcds

a dedicated server is pretty easy to set up anyway, so there’s no reason not to set one up for testing.

buut, other than the 2 second timer workaround, is the rest of the script acceptable?
Or is there something obvious I’m doing ‘wrong’? (for example, my use of local functions, timer names, variables, etc?)

I don’t think you need to redefine your functions every time ITEM:OnBuy is called - did you do that for a reason? If not, that’s something you can improve.

It’s better practice to use srcds. You’ll come across issues eventually caused by using a listen server, and it’ll take you a fair while to fix it.

Thank you for the tip!

I guess the reason why I’m defining it in there because I wasn’t sure how I could use the “ply” entity from the OnBuy function if I defined my own functions at the beginning.

Example:



function MyFunction()
    ply:setWalkSpeed(500)
end

GM:PlayerSpawn(ply)
    MyFunction()
end

^ I have a feeling that’s incorrect and would cause an error?

hook.Add( “PlayerSpawn”, “SomeUniqueIdentifyingNameForThisHook”, function( ply )

ply:SetWalkSpeed( 500 );

end );

You can pass variables (including player objects) as parameters to functions.

For example:

[lua]function MyFunction(ply)
ply:SetWalkSpeed(500)
end

function GM:PlayerSpawn(ply)
MyFunction(ply)
end[/lua]

So the variable ply that is passed into GM:PlayerSpawn by Garry’s Mod is then passed into your function MyFunction.

Thank you Flapadar, that’s good to know.

I’m used to working with another programming language where functions are created quite differently. Sometimes I think I’d have an easier time with lua if I didn’t know that other language :p.