Is this a good server-sided click destroy?

Help and Feedback Code Review
#1

I decided to get back into LUA and so I wanted to start working on a few projects to jog my mind.
It’s a very basic click-destroy that uses remote events/functions and a tool to function.

How it works?

The player has a tool in there inventory. When equipped, it will send the server the target of the mouse. The server then checks if the target is nil (pointing at something that isnt a part / the sky) and if it is on a blacklist (parts you cant destroy.)

If so, it returns true and sends these results back to the client. Once the client gets the all good, it then begins listening for the event that destroys the part.

Note: You can’t fire the destroy part event without going through the checking event, and if you do you get kicked by the server. The activation event wont start until after you’ve passed the check.

Next, the server then finally checks if you already passed the check and the part is safe to destroy. If so, the server destroys the part. Simple!

Server Script

--// Variables \\--
local rs, players = game:GetService("ReplicatedStorage"), game:GetService("Players")
local requestDestroy = rs.RequestEvent
local checkPart = rs.CheckPart

local allowedToFire = false
local selectedTarget
local remoteFired = false
local blacklist = {
	"Baseplate"
};
--// Remotes \\--

checkPart.OnServerInvoke = function(player, target)
	
	remoteFired = true
	allowedToFire = false
	selectedTarget = nil
	
	if target == nil then
		print("Selected target is nil.")
		allowedToFire = false
		return 
	else
		for i, v in pairs(blacklist) do
			if target.Name ~= v then
				print("Target not in blacklist.")
				game.Workspace[player.Name].Sword.Handle.Color = Color3.new(0.0666667, 1, 0.141176)
				print("Set handle to green.")
				allowedToFire = true
				selectedTarget = target
				return true
			else
				allowedToFire = false
				return false
			end
		end
	end
	game.Workspace[player.Name].Sword.Handle.Color = Color3.new(1, 0, 0.0156863)
	remoteFired = false
end

requestDestroy.OnServerEvent:Connect(function(player, target)
	if not remoteFired then
		player:Kick("Attempting to illegally fire remotes.")
	else
		if selectedTarget == nil then
			return
		else
			selectedTarget:Destroy()
		end
	end
end)

Local Script

--// Variables \\--
local rs, players, runService = game:GetService("ReplicatedStorage"), game:GetService("Players"), game:GetService("RunService")
local requestDestroy = rs.RequestEvent
local checkPart = rs.CheckPart
local player = players.LocalPlayer
local tool = script.Parent
local mouse = player:GetMouse()

local debounce = false

--// Events \\--

runService.RenderStepped:Connect(function()
	if not debounce then
		debounce = true 
		
		tool.Equipped:Connect(function()
			print("Tool is equipped.")
			local requestAllowed = checkPart:InvokeServer(mouse.Target)
		
			if requestAllowed then
				print("Beginning to listen for mouse input.")
				tool.Activated:Connect(function()
					requestDestroy:FireServer(mouse.Target)
				end)
			end
		end)
	end
end)

I made sure to always keep in mind not to trust the client and therefore I minimized how much control the client has when it comes to allowing the parts to be destroyed. If you can bypass this or point out any flaws, that would be nice.

I was also trying to figure out how to actively get the player’s mouse target while its equipped, but gave up. If you know how please let me know.

Game: Block Mover Test - Roblox

1 Like
#2

The blacklist check is broken. Since both branches return it will only iterate over the first element on the list before returning.

Also, you never check if the remote is being fired by someone who possesses the tool. This allows other clients to manually fire the remotes even if they don’t have it.

3 Likes
#3

When multiple players use this type of setup I believe you will get lag. I did this client-server-client setup and ran into problems. I would suggest updating the client first, sending their information to the server, checking it, and then send the information to everyone but the client that sent it.

1 Like
#4

Hello Synthexis, and welcome back to Lua.

There were a few problems I saw with your code. Let’s start with with the local script.

  1. I am not quite sure why you have a Debounce in a RunService function. If you don’t want the function to run again you can simply just do
local Connection

Connection = runService.RenderStepped:Connect(function()
      Connection:Disconnect()

This Disconnects the Event, not making the function fire anymore. A Connection returns a connection object, which we can do :Disconnect() on. I also don’t see the need to put all of that in a RenderStepped event if it’s simply going to run once.

Lets now move onto the Server Script :

  1. I normally like to keep some order when declaring variables for readability purposes; you could order them like :
 -- Services --
local rs, players = game:GetService("ReplicatedStorage"), game:GetService("Players")
-- Objects --
local requestDestroy = rs.RequestEvent
local checkPart = rs.CheckPart
-- Tables --
local blacklist = {
	"Baseplate"
};
-- Booleans --
local allowedToFire = false
local remoteFired = false
-- Nil --
local selectedTarget
  1. In your loop checking the blacklists, pairs is meant to iterate over Dictionaries/Hash Tables, while ipairs is meant to iterate over arrays, an example :
local Dictionary = {
   onedar = "Programmer"
}
-- pairs -- 

local Array = {
    "Programmer" -- When no key is declared in the table, the table is an array 
}
-- ipairs --

The main reason of this is because of their source code.

-- pairs -- 
function pairs(t)
   return next, t, nil
end

-- ipairs -- (https://www.lua.org/pil/7.3.html)
function iter (a, i)
     i = i + 1
     local v = a[i]
   if v then
     return i, v
   end
end

function ipairs (a)
      return iter, a, 0
end

  1. Another problem in your for loop is the lack of use of “". "” is a throwaway variable used to show that the value that it retains, will not be used in the program. It’s always a good idea to use “_” where you can.

  2. One little issue with your code is the utilization of game.Workspace, roblox added an environmental variable : ‘‘workspace’’, this gets the service workspace. I suggest you use it.

  3. In your final function (this is optional), you have a lot of if statements. You can reduce those with logical operators :

return ( not remoteFired and player:Kick("") ) or ( selectedTarget == nil and nil ) or selectedTarget:Destroy()

Overall, great script. Happy Holidays :+1:

1 Like