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!
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!
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!
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:
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.
Fixed the condition hit.Parent:IsA(hum) to hit.Parent:IsA("Model") and hum in the CheckOwner function.
Used game.Players:FindFirstChild to find players instead of concatenating names. This ensures safer player detection and prevents potential errors.
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.
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:
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
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).
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.
Here are some suggestions to improve your Tycoon Door System script:
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.
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().
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.
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.
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.
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.