Improving the server-side of a goods delivery system

[SovereignFrost]

I have worked on a server-side of a goods delivery system that will allow the player to deliver goods to different locations for a cash reward. Other players can target this player for a smaller reward.
I am currently unhappy with the amount of code I have used and believe there might be better practices I can use to achieve the same results.

I have added comments to some of the code but feel most can be slightly self-explanatory however if you need any further explaining of what some parts do let me know and I’ll edit the post to include more.

Rbxl file: CodeReview2.rbxl (24.3 KB)

Latest plain text code:

--[[
	
	Name: Smuggle System
	Description: To allow players to deliver different items to random locations for rewards
	
	Created: 13/10/19	
	Writer: SovereignFrost
	
--]]


local module = {}
local debounces = {}

local replicated = game:GetService('ReplicatedStorage')
local players = game:GetService('Players')

local shared_modules = replicated.Shared.Modules
local smuggle_module = require(shared_modules.Activities.Smuggle)
local award_stat  = require(shared_modules.AwardStat)
local teleport_player = require(shared_modules.TeleportPlayer)

local tagModel = script.TagModel
local REMOTE_DEBOUNCE_TIME = 3

module.RemoveRemoteDebounce = function(player)
	delay(REMOTE_DEBOUNCE_TIME, function()
		debounces[player] = nil
	end)
end
module.JobCooldown = function(player, job) -- Each job has their own unique cooldown
	delay(smuggle_module.JOB_PAY_INCREASE[job].cooldown, function()
		player.Cooldowns[job].Value = false
	end)
end
module.DistanceCheck = function(part1, part2, maxDistance)
	if maxDistance then
		return (part1.Position - part2.Position).magnitude < maxDistance
	end
	return (part1.Position - part2.Position).magnitude 
end

module.CanContinue = function(player, job)
	local SHOULD_CHECK_IF_BY_SPAWN = smuggle_module.SPAWN_CHECK
	local SHOULD_CHECK_IF_VALID_TEAM = smuggle_module.TEAM_CHECK
	local hrt = player.Character:FindFirstChild('HumanoidRootPart')
	
	local function teamCheck()
		local VALID_TEAMS = smuggle_module.WHITELISTED_TEAMS
		for _, _team in next, VALID_TEAMS do
			local whitelisted
			if player.Team.Name == _team then
				whitelisted = true
			end
			if not whitelisted then
				return false
			end
		end
	end
	local function spawnCheck()
		local smuggleStart = smuggle_module.spawns:FindFirstChildOfClass('Part')
		local distance = module.DistanceCheck(hrt, smuggleStart, smuggle_module.SPAWN_MAX_DISTANCE)
		if not distance then
			return false
		end
	end
	
	if not debounces[player] then
		if SHOULD_CHECK_IF_BY_SPAWN then
			return spawnCheck()
		end
		if SHOULD_CHECK_IF_VALID_TEAM then
			return teamCheck()
		end
		if player.GlobalChecks.JobActivated.Value == false and player.Cooldowns[job].Value == false then		
			debounces[player] = true
			return true
		end
	end
end
module.ActivateGodmode = function(player, Time)
	local originalMax = player.Character.Humanoid.MaxHealth
	player.Character.Humanoid.MaxHealth = math.huge
	player.Character.Humanoid.Health = math.huge
	
	-- Temp god mode so that the player is not killed during the cutscene or 
	-- right as they begin
			
	delay(Time, function()
		player.Character.Humanoid.MaxHealth = originalMax
		player.Character.Humanoid.Health = originalMax
	end)
end
module.OnServerInvoke = function(player, args)
	local contract_selected = args.contract
	local contract_settings = smuggle_module.JOB_PAY_INCREASE[contract_selected]
	if not contract_settings then
		return false
	end
	if not player.Cooldowns:FindFirstChild(contract_selected) then
		local new = Instance.new('BoolValue', player.Cooldowns)
		new.Name = contract_selected
	end
	if ( module.CanContinue(player, contract_selected)) and args.functionFire == 'BeginSmuggle' then
		delay(0, function()
			module.RemoveRemoteDebounce(player)
		end)		
		local leaderstats = player:FindFirstChild('Stats')
		
		
		--[[
			Ensure the player has enough money to buy into the contract
		--]]
		
		if (leaderstats.Dollars.Value < contract_settings.buy_in) or not contract_settings then 
			return false
		end
		if not player.Cooldowns:FindFirstChild(contract_selected) then
			local new = Instance.new('BoolValue', player.Cooldowns)
			new.Name = contract_selected
		end
		
		module.ActivateGodmode(player, 5)
			
		--[[
			Find a random destination from a list of points to send the player to
			deliver the package
		--]]
		
		local myEvent = player:FindFirstChildOfClass('RemoteEvent')
		local smuggleStart = smuggle_module.spawns:FindFirstChildOfClass('Part')
		local points = smuggle_module.points:GetChildren()
		local destination = Random.new():NextInteger(1, points)
		local hrt = player.Character:FindFirstChild('HumanoidRootPart')
		
		if destination then
			teleport_player.TeleportPlayer(player, smuggleStart.Position, nil, 1)
			player.GlobalChecks.JobActivated.Value = true
						
			leaderstats.Dollars.Value = leaderstats.Dollars.Value - contract_settings.buy_in 
			local notificationArgs = {
				_text = {title = 'SMUGGLE MISSION!', subText = 'Smuggle the package without detection to the drop off zone'},
				_type = 'TopMiddle',
				_time = 5,
				_sound = 'MissionBegin'
			}
			local maxDeliverTime = smuggle_module.MAX_DELIVERY_TIME
			local timeLeft = maxDeliverTime
			myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
			myEvent:FireClient(player, {action = 'DirectPlayerTo', _tbl = {location = destination, maxDistanceApart = 5, maxDirectionTime = timeLeft}})
			myEvent:FireClient(player, {action = 'SmuggleMissionBegin', timer = timeLeft, destination = destination})
		
			local Inside
			local INTERVAL = 1
			
			local function Failed()
				local notificationArgs = {
					_text = {title = 'SMUGGLE MISSION!', subText = 'MISSION FAILED!'},
					_type = 'TopMiddle',
					_time = 3,
					_sound = 'MissionBegin'
				}
				myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
			end
			local function Success()
				local notificationArgs = {
					_text = {title = 'SMUGGLE MISSION!', subText = 'CRATE DELIVERED!'},
					_type = 'TopMiddle',
					_time = 3,
					_sound = 'MissionBegin'
				}
				myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
					
				local timeTook   = maxDeliverTime - timeLeft
				local JOB_PAY_INCREASE = smuggle_module.JOB_PAY_INCREASE
					
				local CASH = function(job, _time, _distance)
					return JOB_PAY_INCREASE[job].cash --*  (_distance / _time)
				end
					
				local xpReward   = JOB_PAY_INCREASE[contract_selected].xp
				local cashReward = CASH(contract_selected, timeTook, (smuggleStart.Position - destination.Position).magnitude)
					
				award_stat.AwardXP(player, {amount = xpReward})
				award_stat.AwardCash(player, {amount = cashReward})
			end
			--[[
				Check if the player is inside a certain location by a magnitude radius
			--]]
			local function inRadius(args, amount, printData)
				--[[
					The amount value is to prevent local teleports by the client to cheat,
					a serverside anti exploit will push them back preventing them from getting
					a successful inRadius check. 
				--]]
				local _hrt = args.hrt
				local _destination = args.destination
				local _maxDistance = args.maxDistance
					
				if not _destination or not _hrt then
					return true
				end
				if printData then
					print( ( _hrt.Position - _destination.Position ).magnitude <= _maxDistance )
				end
				if (_hrt.Position - _destination.Position).magnitude <= _maxDistance then --smuggle_module.POINT_MAX_DISTANCE then
					if amount <= 0 then
						return true
					else
						wait(1)
						return inRadius(args, amount - 1)
					end
				end
			end
				
			local function collectDogtag(tag, copyHRT, killer, bindableEvent)
				local TimeToReachBackpack = 15
				local TouchConnection
				local Found
				
				TouchConnection = tag.Dogtags.Touched:Connect(function(obj)
					local touchPlr = players:GetPlayerFromCharacter(obj.Parent)
					if touchPlr and touchPlr == killer then
						Found = true
						TouchConnection:Disconnect()
					end
				end)
				while true do
					wait(1)
					TimeToReachBackpack = TimeToReachBackpack -1
					if TimeToReachBackpack <= 0 then
						print('Didnt reach in time')
						copyHRT:Destroy()
						tag:Destroy()
						break
					end
					if Found then
						print(killer.Name .. ' has collected the killtag')
						copyHRT:Destroy()
						tag:Destroy()
										
						award_stat.AwardXP(killer, {amount = 20})
						award_stat.AwardCash(killer, {amount = 50})
										
						local notificationArgs = {
							_text = {title = 'SMUGGLE MISSION!', subText = 'CRATE DESTROYED!'},
							_type = 'TopMiddle',
							_time = 3,
							_sound = 'MissionBegin'
						}
						myEvent:FireClient(killer, {action = 'SendNotification', _tbl = notificationArgs})
						break
					end
					bindableEvent:Fire(1)
				end
			end	
			
			local charLoaded = player.GlobalChecks.CharacterLoads
			local startCharLoaded = charLoaded.Value
			
			local creator_tag
			local died = false
			
			player.Character.Humanoid.ChildAdded:Connect(function(obj)
				if obj.Name == 'creator' then
					creator_tag = obj
				end
			end)
			player.Character.Humanoid.Died:Connect(function()
				died = true
			end)
						
			while true do
				wait(INTERVAL)
				--print(timeLeft)
				timeLeft = timeLeft - INTERVAL
				
				if not player or not player.Parent then
					break
				elseif timeLeft <= 0 or not player.Character or charLoaded.Value > startCharLoaded then
					print('Failed')
					Failed()
					break
				elseif died or player.Character.Humanoid.Health <=0 then
					delay(0, function()
						Failed()
					end)
					print('Dead')
					if creator_tag then
						print('Found creator')
						if creator_tag.Value then
							print(creator_tag.Value)
							local owner = creator_tag.Value
							local function lockModel(model)
								for _, v in next, model:GetDescendants() do
									if v:IsA('BasePart') then
										v.Anchored = true
									end
								end
							end
							--[[
								Set a "dogtag" model to where the player was eliminated from
								and check if the killer collects it to receive his reward
							--]]
							local character = owner.Character
							local copyHRT = player.Character.HumanoidRootPart:Clone()
							lockModel(copyHRT)
							copyHRT.Parent = workspace
							copyHRT.Transparency = 0.5
							copyHRT.BrickColor = BrickColor.new('Really red')
							
							local tag = tagModel:Clone()
							tag:SetPrimaryPartCFrame(copyHRT.CFrame)
							tag.Parent = workspace
							print('Created tag..')
							
							local bindableEvent = Instance.new('BindableEvent')
							collectDogtag(tag, copyHRT, owner, bindableEvent)
							local response = bindableEvent.Event:Wait()
							if response == 1 then
								bindableEvent:Destroy()
								break
							end
						end
					end
					break
				else
					delay(0, function()
						local tbl = {hrt = hrt, destination = destination, maxDistance = smuggle_module.POINT_MAX_DISTANCE}
						if inRadius(tbl, 1.8) then
							Inside = true
						end
					end)
					if Inside and timeLeft > 0 then
						Success()
						print('Success!')
						break
					end
				end
			end
			player.Cooldowns[contract_selected].Value = true
			print('Time to delete the contract')
			myEvent:FireClient(player, {action = 'SmuggleMissionOver'})
			player.GlobalChecks.JobActivated.Value = false
			module.JobCooldown(player, contract_selected)
		else
			error('Issue finding destination from module.RandomChild')
		end
	end
end

return module

Plain text code 1.0:

Summary

local module = {}
local debounces = {}

local replicated = game:GetService('ReplicatedStorage')
local shared_modules = replicated.Shared.Modules
local smuggle_module = require(shared_modules.Activities.Smuggle)
local TagModel = script.TagModel

module.DeleteDebounce = function(player)
	delay(3, function()
		debounces[player] = nil
	end)
end
module.DeleteContractDebounces = function(player, job)
	delay(smuggle_module.JOB_PAY_INCREASE[job].cooldown, function()
		player.Cooldowns[job].Value = false
	end)
end
module.RandomChild = function(Table)
	return Table[math.random(#Table)]
end
module.CanContinue = function(player, job)
	local CHECK_IF_BY_SPAWN = smuggle_module.SPAWN_CHECK
	local CHECK_IF_CORRECT_TEAM = smuggle_module.TEAM_CHECK
	if not debounces[player] then
		local hrt = player.Character:FindFirstChild('HumanoidRootPart')
		if CHECK_IF_BY_SPAWN then
			local smuggleStart = smuggle_module.spawns:FindFirstChildOfClass('Part')
			if (hrt.Position - smuggleStart.Position).magnitude >  smuggle_module.SPAWN_MAX_DISTANCE then
				return false
			end
		end
		if CHECK_IF_CORRECT_TEAM then
			local VALID_TEAMS = smuggle_module.WHITELISTED_TEAMS
			for _, _team in next, VALID_TEAMS do
				local whitelisted
				if player.Team.Name == _team then
					whitelisted = true
				end
				if not whitelisted then
					return false
				end
			end
		end
		if player.GlobalChecks.JobActivated.Value == false and player.Cooldowns[job].Value == false then		
			debounces[player] = true
			return true
		end
	end
end
module.OnServerInvoke = function(player, args)
	local contract_selected = args.contract
	local contract_settings = smuggle_module.JOB_PAY_INCREASE[contract_selected]
	if not contract_settings then
		return false
	end
	if not player.Cooldowns:FindFirstChild(contract_selected) then
		local new = Instance.new('BoolValue', player.Cooldowns)
		new.Name = contract_selected
	end
	if ( module.CanContinue(player, contract_selected)) and args.functionFire == 'BeginSmuggle' then
		delay(0, function()
			module.DeleteDebounce(player)
		end)		
		local leaderstats = player:FindFirstChild('Stats')
		local awardStat  = require(shared_modules.AwardStat)
		
		--[[
			Ensure the player has enough money to buy into the contract
		--]]
		if leaderstats.Dollars.Value < contract_settings.buy_in then 
			return false
		end
		if not contract_settings then
			return false
		end
		if not player.Cooldowns:FindFirstChild(contract_selected) then
			local new = Instance.new('BoolValue', player.Cooldowns)
			new.Name = contract_selected
		end
		
		--[[
			Find a random destination from a list of points to send the player to
			deliver the package
		--]]
		local myEvent = player:FindFirstChildOfClass('RemoteEvent')
		local smuggleStart = smuggle_module.spawns:FindFirstChildOfClass('Part')
		local destination = module.RandomChild(smuggle_module.points:GetChildren())
		local hrt = player.Character:FindFirstChild('HumanoidRootPart')
		if destination then
			local originalMax = player.Character.Humanoid.MaxHealth
			
			player.Character.Humanoid.MaxHealth = math.huge
			player.Character.Humanoid.Health = math.huge
                        -- Temp god mode so that the player is not killed during the cutscene or right as they begin
			
			delay(5, function()
				player.Character.Humanoid.MaxHealth = originalMax
				player.Character.Humanoid.Health = originalMax
			end)
			
			local tp = require(shared_modules.TeleportPlayer)
			tp.TeleportPlayer(player, smuggleStart.Position, nil, 1)
			player.GlobalChecks.JobActivated.Value = true
						
			leaderstats.Dollars.Value = leaderstats.Dollars.Value - contract_settings.buy_in 
			local notificationArgs = {
				_text = {title = 'SMUGGLE MISSION!', subText = 'Smuggle the package without detection to the drop off zone'},
				_type = 'TopMiddle',
				_time = 5,
				_sound = 'MissionBegin'
			}
			local maxDeliverTime = smuggle_module.MAX_DELIVERY_TIME
			local timeLeft = maxDeliverTime
			myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
			myEvent:FireClient(player, {action = 'DirectPlayerTo', _tbl = {location = destination, maxDistanceApart = 5, maxDirectionTime = timeLeft}})
			myEvent:FireClient(player, {action = 'SmuggleMissionBegin', timer = timeLeft, destination = destination})
		
			local Inside
			local INTERVAL = 1
			
			local function Failed()
				local notificationArgs = {
					_text = {title = 'SMUGGLE MISSION!', subText = 'MISSION FAILED!'},
					_type = 'TopMiddle',
					_time = 3,
					_sound = 'MissionBegin'
				}
				myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
			end
			local function Success()
				local notificationArgs = {
					_text = {title = 'SMUGGLE MISSION!', subText = 'CRATE DELIVERED!'},
					_type = 'TopMiddle',
					_time = 3,
					_sound = 'MissionBegin'
				}
				myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
					
				local timeTook   = maxDeliverTime - timeLeft
				local JOB_PAY_INCREASE = smuggle_module.JOB_PAY_INCREASE
					
				local CASH = function(job, _time, _distance)
					return JOB_PAY_INCREASE[job].cash --*  (_distance / _time)
				end
					
				local xpReward   = JOB_PAY_INCREASE[contract_selected].xp
				local cashReward = CASH(contract_selected, timeTook, (smuggleStart.Position - destination.Position).magnitude)
					
				awardStat.AwardXP(player, {amount = xpReward})
				awardStat.AwardCash(player, {amount = cashReward})
				
			end
			--[[
				Check if the player is inside a certain location by a magnitude radius
			--]]
			local function inRadius(args, amount, printData)
				--[[
					The amount value is to prevent local teleports by the client to cheat,
					a serverside anti exploit will push them back preventing them from getting
					a successful inRadius check. 
				--]]
				local _hrt = args.hrt
				local _destination = args.destination
				local _maxDistance = args.maxDistance
					
				if not _destination then
					return true
				end
				if not _hrt then
					return false
				end
				if printData then
					print( ( _hrt.Position - _destination.Position ).magnitude <= _maxDistance )
				end
				if (_hrt.Position - _destination.Position).magnitude <= _maxDistance then --smuggle_module.POINT_MAX_DISTANCE then
					if amount <= 0 then
						return true
					else
						wait(1)
						return inRadius(args, amount - 1)
					end
				end
			end
			
			local charLoaded = player.GlobalChecks.CharacterLoads
			local startCharLoaded = charLoaded.Value
			
			local creator_tag
			local died = false
			
			player.Character.Humanoid.ChildAdded:Connect(function(obj)
				if obj.Name == 'creator' then
					creator_tag = obj
				end
			end)
			player.Character.Humanoid.Died:Connect(function()
				died = true
			end)
						
			while true do
				wait(INTERVAL)
				--print(timeLeft)
				timeLeft = timeLeft - INTERVAL
			
				if timeLeft <= 0 or not player.Character or charLoaded.Value > startCharLoaded then
					print('Failed')
					Failed()
					break
				elseif not player then
					break
				elseif died or player.Character.Humanoid.Health <=0 then
					delay(0, function()
						Failed()
					end)
					print('Dead')
					if creator_tag then
						print('Found creator')
						if creator_tag.Value then
							print(creator_tag.Value)
							local owner = creator_tag.Value
							local function lockModel(model)
								for _, v in next, model:GetDescendants() do
									if v:IsA('BasePart') then
										v.Anchored = true
									end
								end
							end
							
							--[[
								Set a "dogtag" model to where the player was eliminated from
								and check if the killer collects it to receive his reward
							--]]
							local character = owner.Character
							local copyHRT = player.Character.HumanoidRootPart:Clone()
							lockModel(copyHRT)
							copyHRT.Parent = workspace
							copyHRT.Transparency = 0.5
							copyHRT.BrickColor = BrickColor.new('Really red')
							
							local tag = TagModel:Clone()
							tag:SetPrimaryPartCFrame(copyHRT.CFrame)
							tag.Parent = workspace
							
							print('Created tag..')
							
							local Found = false
							local TimeToReachBackpack = 15
							local Players = game:GetService('Players')
							local TouchConnection
							TouchConnection = tag.Dogtags.Touched:Connect(function(obj)
								local touchPlr = Players:GetPlayerFromCharacter(obj.Parent)
								if touchPlr and touchPlr == owner then
									Found = true
									TouchConnection:Disconnect()
								end
							end)
							
							--delay(0, function()
								while true do
									wait(1)
									TimeToReachBackpack = TimeToReachBackpack -1
									if TimeToReachBackpack <= 0 then
										print('Didnt reach in time')
										copyHRT:Destroy()
										tag:Destroy()
										break
									end
									if Found then
										print(owner.Name .. ' has collected the killtag')
										copyHRT:Destroy()
										tag:Destroy()
										
										awardStat.AwardXP(owner, {amount = 20})
										awardStat.AwardCash(owner, {amount = 50})
										
										local notificationArgs = {
											_text = {title = 'SMUGGLE MISSION!', subText = 'CRATE DESTROYED!'},
											_type = 'TopMiddle',
											_time = 3,
											_sound = 'MissionBegin'
										}
										myEvent:FireClient(owner, {action = 'SendNotification', _tbl = notificationArgs})
										break
									end
								end
								break
							--end)
						end
					end
					break
				else
					delay(0, function()
						local tbl = {hrt = hrt, destination = destination, maxDistance = smuggle_module.POINT_MAX_DISTANCE}
						if inRadius(tbl, 1.8) then
							Inside = true
						end
					end)
					if Inside and timeLeft > 0 then
						Success()
						print('Success!')
						break
					end
				end
			end
			player.Cooldowns[contract_selected].Value = true
			print('Time to delete the contract')
			myEvent:FireClient(player, {action = 'SmuggleMissionOver'})
			player.GlobalChecks.JobActivated.Value = false
			module.DeleteContractDebounces(player, contract_selected)
		else
			error('Issue finding destination from module.RandomChild')
		end
	end
end

return module

Thank you for reading

[ThanksRoBama (Emma)]

Here’s a big list of all the things I noticed while reading through your code.

I would add a comment at the top of the script with the name of the script and the purpose of the script, in broad terms.

Inconsistent capitalization and use of ‘_’ in variable names?

module.DeleteDebounce: 3 is magic number, replace with variable. Also, purpose is unclear from name, yet DeleteContractDebounces has more explanation in variable name? Inconsistent level of wordiness.

RandomChild uses math.random, which has worse randomness than Randow.new():NextInteger().

(hrt.Position - smuggleStart.Position).magnitude > smuggle_module.SPAWN_MAX_DISTANCE . You have function for RandomChild, why not for Distance?

Are CHECK_IF_BY_SPAWN and CHECKIF_CORRECT_TEAM bools that control if the check should happen? They seem more like “commands”, which makes me think of functions. I’d change it to SHOULD_CHECK_IF_BY_SPAWN. Also, I would split those checks into separate, well-named functions to make CanContinue less of a mess. Also, after reading CanContinue it’s unclear what it does. I’d add a short comment explaining what it means to “continue”.

if not contract_settings then return false?? Add a comment or warn() call explaining what has gone wrong. Also explain why it returns false instead of nil.

Line 90: if destination then is unecessary, or should be if smuggle_module and RandomChild are correct.

Lines 93-99: are you making the player temporarily invulnerable? That needs to be split into a separate function or have a comment explaining it.

Lines 121-183: Failed(), Success() and inRadius() don’t need to be defined inside OnServerInvoke. Same with localModel at line 221.

Are you sure that a reference to a Player object that has left the game becomes nil? Might want to test that to make sure. If not, replace elseif not player then with elseif not player.Parent then.

Lines 229 to 231: really good comment, this is the stuff that helps the reader unstand WHY you’re doing the things that the code does.

Lines 258 to 287: you could probably split this into its own function.

Other than thay, your code seems correct and somewhat well-organized. Reading it is really hard though, because of the very long OnServerInvoke function with many responsibilites and deep indentation levels. At ~25 LoC it’s not too bad, but still it definitely doesn’t help readability.

Hope this helps, and of course take everything with a grain of salt and subjectivity

[SovereignFrost]

Thank you for the feedback.

module.DeleteDebounce: 3 is magic number, replace with variable. Also, purpose is unclear from name, yet DeleteContractDebounces has more explanation in variable name? Inconsistent level of wordiness

The actual debounce is for the remote to prevent spamming, I’ll consider changing the name of this to be more clear

RandomChild uses math.random, which has worse randomness than Randow.new():NextInteger().

I’ll look into Random.new(), wrote this function before it came out

Also, after reading CanContinue it’s unclear what it does. I’d add a short comment explaining what it means to “continue”.

CanContinue is just a series of checks to make sure the player is allowed to begin a delivery, preventing any exploiters from beginning them when they’re not allowed to. I’ll add a comment to this when I’m on my computer and possibly rename it.

if not contract_settings then return false?? Add a comment or warn() call explaining what has gone wrong. Also explain why it returns false instead of nil.

This is just to make sure the client has sent the correct information, there are three types of deliveries. This is to prevent errors or any incorrect details from appearing later in the script. I have a tendency to use both nil and false to return to the local script while not checking that data anyway.

Lines 93-99: are you making the player temporarily invulnerable? That needs to be split into a separate function or have a comment explaining it.

You’re right, was a quick adjustment I added before I posted the script. I’ll do that when I’m on my computer

Thank you for taking your time to review the code, I’m going to use this to make my code more readable. Do you think there are any better ways I could improve the actual delivery part of the code? I find from where I’m doing multiple while loops inside of each other to be very messy.

[notchloeno (chloe)]

Once you’ve refactored your code in accordance with @ThanksRoBama’s suggestions, it might be worth putting the original code in a spoiler and posting the most recent version so we can help you iterate through it - I’d personally be enthusiastic to as this is the first Code Review for a while that is complex and not just a DataStore2 question

[SovereignFrost]

I have updated the code using the suggestions of @ThanksRoBama, I haven’t been able to test it yet as my internet has been very slow meaning I could not open the full game file.

[notchloeno (chloe)]

@SovereignFrost

if player.Team.Name == _team then
    whitelisted = true
end

This here looks like a magic string, wherein you use comparison of strings in your boolean logic. This is generally a programming faux pas due to the computational complexity of comparing and operating on strings as well as the tendency of compilers to occasionally fiddle around with strings and break code, especially in VMs like Lua uses.

@SovereignFrost

if not distance then
    return false
end

This could be simply replaced with return distance without affecting any logic in the script - minor, but one less conditional and a habit to build. Taking it a step further you could simply return the resuly of module.DistanceCheck() rather than assigning it to the variables distance.

@SovereignFrost

	if not contract_settings then
		return false
	end

Same story here

@SovereignFrost

		if (leaderstats.Dollars.Value < contract_settings.buy_in) or not contract_settings then 
			return false
		end

and here! There are a number more throughout the script but you’ll easily find them if you look for them.

@SovereignFrost

if destination then
    local tp = require(shared_modules.TeleportPlayer)
    tp.TeleportPlayer(player, smuggleStart.Position, nil, 1)
    ...

And require()s should be at the top of your script, so as to avoid cyclical dependencies. This may reduce loading time infinitesimally, but is worth it to potentially save the headache later - these errors aren’t always easy to find.

@SovereignFrost

myEvent:FireClient(player, {action = 'SendNotification', _tbl = notificationArgs})
myEvent:FireClient(player, {action = 'DirectPlayerTo', _tbl = {location = destination, maxDistanceApart = 5, maxDirectionTime = timeLeft}})
myEvent:FireClient(player, {action = 'SmuggleMissionBegin', timer = timeLeft, destination = destination})

Looks like you have a single RemoteEvent/RemoteFunction which is parsed on the other end. This is definitely a valid way of doing things, however I personally find it harder to track later in development, and more complex to write around. Each to their own though.

@SovereignFrost

local Inside

This is nitpicking but I would assign it a Nil value.

@SovereignFrost

bindableEvent:Fire('BREAK')

Looks like magic strings again!

Aside from this, your logic and algorithms are sound, and the code is incredibly self-descriptive with well placed comments explaining anything that needs explaining. You’ve followed programming conventions well and in general your code is brilliant!

[SovereignFrost]

The if then returning was to return the module itself as false if the information given from the client was not valid. If I did return variable then it would return even if it was true.

I’ve moved the require to the top, normally when I edit my code later on I put the variables where I’m coding for convenience then move them to the top. Forgot about that one.

Changed local Inside = false to local Inside

Thank you for the feedback, means a lot. I found my code hard to read after a few weeks of writing it which is why I wanted to clean it up a bit, looks so much better now.