Client to Server Code Review

[umamidayo (dayo)]

• What does the code do and what are you not satisfied with?

I’m not satisfied with the server-sided code because I feel that there are some lines that can be simplified, but can I receive your opinion on this matter?

• What potential improvements have you considered?

I am thinking about moving most of the checks and code to a module, but I also feel that this would over-complicate the fundamentals for a basic interaction script. How would I simplify this code?

• How (specifically) do you want to improve the code?

See above.

Client Script:

local Remotes = game:GetService("ReplicatedStorage"):WaitForChild("Remotes")
local player = game.Players.LocalPlayer
local mouse = player:GetMouse()

mouse.Button1Down:Connect(function()
	Remotes.Mouse_Event:FireServer(mouse.Target)
end)

Server Script:

local SoundService = game:GetService("SoundService")
local Sounds = SoundService.Sounds

local Server_Modules = game.ServerScriptService.Server_Modules
local Replicated_Modules = game.ReplicatedStorage.Modules
local Doors_Module = require(Server_Modules.Doors_Module)
local Util = require(Replicated_Modules.Util)

local Remotes = game.ReplicatedStorage.Remotes

local doorsFolder = workspace.Doors
local doorDebounce = {}

Remotes.Mouse_Event.OnServerEvent:Connect(function(player, target:Instance)
	-- Exploit Checks
	
	if not target then return end
	if (target.Position - player.Character.HumanoidRootPart.Position).Magnitude > 5 then return end
	
	-- Finding Folder
	
	local folder = target:FindFirstAncestorWhichIsA("Folder")
	if not (folder and folder == doorsFolder) then return end
	
	-- Finding Door

	local door = nil
	local startTime = os.time()
	
	repeat
		target = target.Parent
		if target.Parent == doorsFolder then
			door = target
		end
	until door or (os.time() - startTime > 5)
	
	-- Debounce Check
	
	if doorDebounce[door] ~= nil and (tick() - doorDebounce[door]) < 1.1 then return end
	doorDebounce[door] = tick()
	
	-- Lock Check
	
	if not door:GetAttribute("Locked") then Doors_Module.InteractDoor(door) return end
	
	-- Key Check

	local keyName = door:GetAttribute("Key")

	if not keyName then return end

	if Util.GetTool(player, keyName) then
		door:SetAttribute("Locked", false)
		Doors_Module.playSound(door.Body.Base, Sounds.DoorUnlock)
	else
		Doors_Module.playSound(door.Body.Base, Sounds.DoorLocked)
	end
end)

Any feedback is grealy appreciated.

[LeqenDzTR]

do the checks in the local script. This way, you won’t overload the server and there won’t be tons of remote event calls. (the ones that are not exploitable)

[umamidayo (dayo)]

I see, that’s a good idea.

I forgot to mention that I am also connecting the event to another script that checks if the mouse target is an item or not; mainly for picking up tools.

With that in mind, should I rewrite the original script to check for general values (such as distance, target, etc.) and pass them into modules? Your input is greatly appreciated.

[ali432567 (Alizabeth)]

I thought it was just a mouseclick 1 check sorry.

[majdTRM]

Make sure to do the checks both on the client and the server too!

[hbgnhu (CrimKing)]

Checks in the localscript are easily exploitable