Feedback required on my event handler script

Help and Feedback Code Review
#1 
local ServerStorage = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local remoteEventsFolder = ReplicatedStorage.RemoteEvents
local remoteFunctionsFolder = ReplicatedStorage.RemoteFunctions

local tools = ServerStorage.Tools

local toolsInformation = {
	
	Torch = {
	   LevelRequired = 0,
	   Cost = 0,
	};
	
	MedKit = {
	   LevelRequired = 10,
	   Cost = 245,
	};
	
	Crowbar = {
	   LevelRequired = 25,
	   Cost = 400,
	};
	
	Backpack = {
	   LevelRequired = 50,
	   Cost = 750,
	};
	
	Backpack = {
	   LevelRequired = 100,
	   Cost = 1250,
	};
	
}

remoteEventsFolder.Torch.OnServerInvoke = function(player, Target)
	if Target == "TorchEvent" then
	
	if player.leaderstats.Points.Value >= toolsInformation.Torch.Cost and player.leaderstats.Level.Value >= toolsInformation.Torch.LevelRequired and not player.Backpack:FindFirstChild("Torch") then
	
		player.Data.Torch.Owned.Value = true
		player.leaderstats.Points.Value	-= toolsInformation.Torch.Cost.Value
		
		local tool = tools.Torch:Clone()
		tool.Parent = player.Backpack
		tool:Clone()
		tool.Parnet = player.StarterGear
        return true 
	else
		 return false
		end
	end
end
1 Like
#2

I’d split the toolsInformation table into a separate ModuleScript and just require that, so that you can later modify the data without having to look at any of the code that uses the data. That also allows a different script to access the same data later on without having to manually keep two separate versions of the data up-to-date. Plus you don’t have to collapse it in the code editor to avoid having to scroll past it constantly.

Calling the parameter “Target” is bad variable naming. It’s completely unclear what it’s supposed to represent unless you read the code and make some assumptions about what it does. I’d change it to eventType.

toolsInformation.Torch is a table, so you don’t need the extra .Value after Cost.

You can split this huge condition into several lines, to make it a lot more readable. I also wrapped the not in a pair of parentheses, because the operator precedence of not sometimes causes confusion:

return player.leaderstats.Points.Value >= toolInfo.Cost and 
	player.leaderstats.Level.Value >= toolInfo.LevelRequired and 
	(not player.Backpack:FindFirstChild("Torch"))

I’m guessing you’ll want to add code to add make all the other tools purchasable later on? Purchasing a tool is always the same in your case, so you can abstract that out into some separate functions:

How I'd change your script: 

function canPlayerPurchaseTool( player, toolName )
	local toolInfo = toolsInformation[toolName]
	assert(toolInfo ~= nil, string.format([[Tried to purchase a tool called "%s", no tool information exists for a tool with that name.]], toolName))

	return player.leaderstats.Points.Value >= toolInfo.Cost and 
		player.leaderstats.Level.Value >= toolInfo.LevelRequired and 
		(not player.Backpack:FindFirstChild("Torch"))
end

function purchaseTool( player, toolName )
	local toolInfo = toolsInformation[toolName]
	assert(toolInfo ~= nil, string.format([[Tried to purchase a tool called "%s", no tool information exists for a tool with that name.]], toolName))

	layer.leaderstats.Points.Value	-= toolInfo.Cost
	player.Data[toolName].Owned.Value = true

	local tool = tools[toolName]:Clone()
	tool.Parent = player.Backpack
	tool:Clone().Parent = player.StarterGear
end

remoteEventsFolder.Torch.OnServerInvoke = function(player, eventType, ...)
	if eventType == "purchase tool" then
		--Wrap in a pcall because a hacker might send any data to a RemoteFunction and cause this script to erro.
		local success = false
		pcall(function()
			local toolName = select(1, ...)
			if canPlayerPurchaseTool(player, toolName) then
				purchaseTool(player, toolName)
				success = true
			end
		end)
		
		return success
	end

	--All eventTypes should return, causing this to never be reached
	warn(string.format([[Unimplemented eventType "%s" reached.]], eventType))
	return false
end
2 Likes
#3

I’m in the process of changing every script, thank you.

1 Like