Review on Tycoon Door System

Hello everyone. I made a tycoon door system and I’d like you all to check it out. I am pretty new to scripting so expect it to be very messy. The following scripts are both in ServerScriptService.

local doorPart = workspace.DoorPart
local opened = doorPart.Opened.Value
local claimDoor = workspace.Claim
local ownerId = claimDoor.OwnerId
local alreadyClaimed = claimDoor.AlreadyClaimed

game.Players.PlayerAdded:Connect(function(plr)
	claimDoor.Touched:Connect(function(hit)
		if hit.Parent:IsA("Accessory") and alreadyClaimed.Value == false then
			local playerHit = hit.Parent.Parent.Name
			print(tostring(playerHit))
			local userId = game.Players:FindFirstChild(tostring(playerHit)).UserId
			print(userId)
			alreadyClaimed.Value = true
			ownerId.Value = userId
			claimDoor.Transparency = 0.6
			claimDoor.CanCollide = false
		elseif alreadyClaimed.Value == true then
			print("Already Claimed")
			wait(5)
		end
	end)
	local function ToggleDoor()
		if opened == false then
			doorPart.Transparency = 0.4
			doorPart.CanCollide = false
			opened = true
			print("Opened")
		elseif opened == true then
			doorPart.Transparency = 0
			doorPart.CanCollide = true
			opened = false
			print("Closed")
		end
	end

	doorPart.Touched:Connect(function(hit)
		local hum = hit.Parent:FindFirstChild("Humanoid")
		local playerTouched = hit.Parent.Name
		local userId = game.Players:FindFirstChild(tostring(playerTouched)).UserId
		print(userId)
		if opened == false and hit.Parent:IsA(hum) and userId ~= ownerId.Value then
			print("Not Owner")
			hit.Parent:BreakJoints()
		end
	end)
	
	game.ReplicatedStorage.BindableEvent.Event:Connect(ToggleDoor)
end)
local Switch = workspace.Part
local clickDetector = Switch.ClickDetector

clickDetector.MouseClick:Connect(function()
	game.ReplicatedStorage.BindableEvent:Fire()
	print("Clicked")
end)

This took me about an hour and a half to complete because, as I said, I am pretty new to scripting. Thank you for and feedback on improvements I can make!

5 Likes

you could use :GetPlayerFromCharacter() instead when getting the player’s id
so like

local userId = game.Players:GetPlayerFromCharacter(hit.Parent.Parent).UserId
3 Likes
  • Organize your code: Group related code together and separate different sections of your code with comments to make it easier to read and understand.
  • Use descriptive variable names: Use descriptive names for your variables and functions to make it clear what they represent and what they do.
  • Avoid global variables: Avoid using global variables as they can make it difficult to track the flow of data in your code. Instead, use local variables and pass them as arguments to functions as needed.
  • Use consistent indentation: Use consistent indentation throughout your code to make it easier to read and understand the structure of your code.
  • Add error handling: Add error handling to your code to gracefully handle any errors that may occur during runtime.

I hope these suggestions help you improve your Tycoon Door System code!

1 Like

Why are you checking if hit.Parent is a accessory? It should be checking if the part is related to the humanoid/in the same model. And I don’t think there’s a need to constantly be making the name of an object to a string of its already a string. Also, don’t forget to make a script when they leave it unclaimed the door!

2 Likes

My Luau/Lua Trained AI using Python gave me this. “uses GPT-4”
I figure this could help let me know if you have any questions about it and ill go through it in studio myself.

Let’s go through each script and make improvements as needed.

Script 1 (ServerScriptService):

local doorPart = workspace.DoorPart
local opened = doorPart.Opened.Value
local claimDoor = workspace.Claim
local ownerId = claimDoor.OwnerId
local alreadyClaimed = claimDoor.AlreadyClaimed

game.Players.PlayerAdded:Connect(function(plr)
    local function ToggleDoor()
        if opened == false then
            doorPart.Transparency = 0.4
            doorPart.CanCollide = false
            opened = true
            print("Opened")
        else
            doorPart.Transparency = 0
            doorPart.CanCollide = true
            opened = false
            print("Closed")
        end
    end

    local function ClaimDoor(hit)
        if hit.Parent:IsA("Accessory") and alreadyClaimed.Value == false then
            local playerHit = hit.Parent.Parent.Name
            print(tostring(playerHit))
            local player = game.Players:FindFirstChild(tostring(playerHit))
            if player then
                local userId = player.UserId
                print(userId)
                alreadyClaimed.Value = true
                ownerId.Value = userId
                claimDoor.Transparency = 0.6
                claimDoor.CanCollide = false
            end
        elseif alreadyClaimed.Value == true then
            print("Already Claimed")
            wait(5)
        end
    end

    local function CheckOwner(hit)
        local hum = hit.Parent:FindFirstChild("Humanoid")
        local playerTouched = hit.Parent.Name
        local player = game.Players:FindFirstChild(tostring(playerTouched))
        local userId = player and player.UserId

        if opened == false and hit.Parent:IsA("Model") and hum and userId ~= ownerId.Value then
            print("Not Owner")
            hum:BreakJoints()
        end
    end

    doorPart.Touched:Connect(CheckOwner)
    claimDoor.Touched:Connect(ClaimDoor)
    game.ReplicatedStorage.BindableEvent.Event:Connect(ToggleDoor)
end)

Revisions made:

  1. Moved the ToggleDoor, ClaimDoor, and CheckOwner functions inside the PlayerAdded event handler. This ensures that each player gets their own set of functions and variables.

  2. Fixed the condition hit.Parent:IsA(hum) to hit.Parent:IsA("Model") and hum in the CheckOwner function.

  3. Used game.Players:FindFirstChild to find players instead of concatenating names. This ensures safer player detection and prevents potential errors.

  4. Added a check if the player is found before accessing the UserId property to avoid errors.

Script 2 (ServerScriptService):

local Switch = workspace.Part
local clickDetector = Switch.ClickDetector

clickDetector.MouseClick:Connect(function()
    game.ReplicatedStorage.BindableEvent:Fire()
    print("Clicked")
end)

The second script seems fine and doesn’t require any revisions.

1 Like

Why are you connecting to game.Players.PlayerAdded? The code can run fine without it.


Improvements you can do:

There's a few things you can do:
  1. switch the if statements and switch to an early return. This is known as a guard clause.
Example

Instead of:

if hit.Parent:IsA("Accessory") and alreadyClaimed.Value == false then
			local playerHit = hit.Parent.Parent.Name
			print(tostring(playerHit))
			local userId = game.Players:FindFirstChild(tostring(playerHit)).UserId
			print(userId)
			alreadyClaimed.Value = true
			ownerId.Value = userId
			claimDoor.Transparency = 0.6
			claimDoor.CanCollide = false
		elseif alreadyClaimed.Value == true then
			print("Already Claimed")
			wait(5)
		end

Do:

if alreadyClaimed.Value then
	print("Already Claimed!")
	task.wait(5)
	return
end

if not hit.Parent:IsA("Accessory") then
    return
else

local playerHit = hit.Parent.Parent.Name
local userId = game.Players:FindFirstChild(tostring(playerHit)).UserId
alreadyClaimed.Value = true
ownerId.Value = userId
claimDoor.Transparency = 0.6
claimDoor.CanCollide = false
  1. Instead of finding the player’s name through a string, you can use the Players service to get the player directly using Players:GetPlayerFromCharacter(Character).
  2. Follow the Roblox Lua Style Guide for naming conventions and organizing your code.
  3. I don’t really know what to write here, but you can shorten the toggleDoor() function like this:
Previous Function
local function ToggleDoor()
		if opened == false then
			doorPart.Transparency = 0.4
			doorPart.CanCollide = false
			opened = true
			print("Opened")
		elseif opened == true then
			doorPart.Transparency = 0
			doorPart.CanCollide = true
			opened = false
			print("Closed")
		end
	end
Newer Function

directly set the properties using the opened variable, or use the ternary operator or variable = if (condition) then 1 else 0 on the properties like this:

local function ToggleDoor()
	if opened == false then
		opened = true
	elseif opened == true then
		opened = false
	end

	doorPart.Transparency = if opened then 0.4 else 0
	doorPart.CanCollide = not opened
end

But now as you can notice, we’re just reversing the opened booleon (booleon is a true or false variable), so we can shorten it as opened = not opened

And if we follow step 3 and rename the door part and claim door as constants, since they don’t ever change, you get this:

local function toggleDoor()
	opened = not opened

	DOOR_PART.Transparency = if opened then 0.4 else 0
	DOOR_PART.CanCollide = not opened
end

Full code with the improvements

Improved code
local Players = game:GetService("Players")

local DOOR_PART = workspace.DoorPart
local CLAIM_DOOR = workspace.Claim

local opened = DOOR_PART.Opened.Value
local ownerId = CLAIM_DOOR.OwnerId
local alreadyClaimed = CLAIM_DOOR.AlreadyClaimed

local function toggleDoor()
	opened = not opened

	DOOR_PART.Transparency = if opened then 0.4 else 0
	DOOR_PART.CanCollide = not opened
end

CLAIM_DOOR.Touched:Connect(function(hit: BasePart)
	local humanoid = hit.Parent:FindFirstChild("Humanoid")
	if not humanoid then --check it's a character
		return
	end
	
	if alreadyClaimed then
		print("Already Claimed!")
		task.wait(5)
		return
	end

	local player = Players:GetPlayerFromCharacter(hit.Parent)
	local userId = player.UserId

	alreadyClaimed.Value = true
	ownerId.Value = userId
	CLAIM_DOOR.Transparency = 0.6
	CLAIM_DOOR.CanCollide = false
end)

DOOR_PART.Touched:Connect(function(hit)
	local humanoid = hit.Parent:FindFirstChild("Humanoid")
	if not humanoid then
		return
	end

	local player = Players:GetPlayerFromCharacter(hit.Parent)
	local userId = player.UserId
	
	if not opened and userId ~= ownerId.Value then
		hit.Parent:BreakJoints()
	end
end)

game.ReplicatedStorage.BindableEvent.Event:Connect(toggleDoor)

But wait, what about the other script?
It works perfectly fine, as it is very short and doesn’t have anything to improve upon.

Edit: I made a mistake, corrected now

1 Like

Here are some suggestions to improve your Tycoon Door System script:

  1. Use consistent variable naming: It’s a good practice to use consistent variable naming conventions. For example, you can use lowercase for local variables and camel case for global variables. This helps improve code readability.
  2. Separate functionality into functions: Instead of having all the code in the PlayerAdded event handler, consider separating the functionality into different functions. This will make the code more modular and easier to read and maintain. You can have functions like ClaimDoor(), ToggleDoor(), and CheckOwnership().
  3. Avoid unnecessary repetitions: In the ToggleDoor() function, you can simplify the code by directly assigning the opposite value to the opened variable. Instead of using an if-else statement, you can use the not operator.
  4. Error handling: It’s essential to handle potential errors that may occur during script execution. For example, when finding a player by name using game.Players:FindFirstChild(), the player might not be found, resulting in a nil value. You should add error handling to handle such cases.
  5. Use RemoteEvents for better security: Currently, you’re using a BindableEvent to toggle the door. However, this can be easily exploited by exploiting the client-side. It’s recommended to use RemoteEvents to handle server-client communication for better security.
  6. Use comments: Adding comments to your code can help explain the purpose of different sections and make it easier for others to understand your code.
1 Like