How Can I Improve My Code?

Help and Feedback Code Review
, , , ,
#1

Robbery system:

  1. First you trigger a part (proximity prompt)

  2. In Serverstorage: 2 values change: InProgress = true and ByWho = playerObject

  3. A Remote-Function invokes the client:
    the client first has a textlabel, wich counts from 10 to 0 on the players screen,
    if its finished, it will return true.

  4. Now it returned true because the counting is over, robbery will now end on the server

The System works.

Server Script:

--[[
	- Start robbery
	- Send a request to client to count down from 10 to 0 on GUI
	- Client counts in gui from 10 to 0
	- If client finished counting, return true (robbery is now over)
	- Now in the server, end the robbery
--]]



-- // Variables
local triggerPart = workspace.TriggerPart
local prox = triggerPart.ProximityPrompt

local inProgress = game:GetService("ServerStorage").BankRobbery.InProgress
local byWho = game:GetService("ServerStorage").BankRobbery.ByWho

local event = game:GetService("ReplicatedStorage").RemoteFunctions.BankRobbery.ShowText



-- // Main Function
prox.Triggered:Connect(function(plr)
	
	local cash = plr:FindFirstChild("leaderstats"):FindFirstChild("Cash")
	
	
	if inProgress.Value == false then
		
		inProgress.Value = true -- Robbery is now in progress
		byWho.Value = plr       -- Show the name of player who is robbing
		
		
		local finished = event:InvokeClient(plr) -- Send request to client
		
		if finished then -- If the robbery is over then
			
			-- End Robbery:
			inProgress.Value = false
			byWho.Value = nil
			
			cash.Value += 100 -- Give the Player a Bonus
			
			print(plr.Name.. " Successfully robbed a bank!")
		end
		
	end
	
end)

**Client Script: **

--[[
	- If function gets invoked, do this:
	
	- Set progressText to visible
	- The Textlabel will count from 10 to 0
	- Set progressText to not visible
	- Return true (The server knows that the counting is over)
--]]



local plr = game:GetService("Players").LocalPlayer
local char = plr.Character or plr.CharacterAdded:Wait()
local hum = char:WaitForChild("Humanoid")

local plrGui = plr.PlayerGui
local progressText = plrGui:WaitForChild("BankRobberyGui"):WaitForChild("ProgressText")

local event = game:GetService("ReplicatedStorage").RemoteFunctions.BankRobbery.ShowText


event.OnClientInvoke = function()
	
	progressText.Visible = true
	
	for i = 10, 0, -1 do
		progressText.Text = "Robbery in progress... " .. i
		print(i)
		task.wait(1)
	end
	
	progressText.Visible = false
	
	return true
	
end

I would greatly appreciate it if an experienced scripter could review this code and provide feedback on how I can improve it.

1 Like
#2

First of all, idk why would you like to use object values, instead of unassigned variable, another very important thing is that you use remote function in a wrong way, see you need to make a lot of checks like timeouts and information handler to make this function actually safe

Thing you can do is use remote event instead, send info for the client to count from 0 to 10, but server should count from 0 to 10 also, you can add os.clock time travel sync to make timer the same for all clients

2 Likes