This week I started a basic building system, to the best of my testing I believe it works with no bugs, but I am a newbie scripter and this is one of my first scripts so I know that I have done some not so good practice in here somewhere and I have come here to ask for tips/feedback on what I should improve on in either general or on this specific script. Any help is appreciated!
wow this is a lot neater than my code, for some reason I tend to put my code in no order so imma try to stick to a format from now on. Also I figured out that instead of:
There’s definitely some question of personal taste in this, so I just want to share some different thoughts on it. I don’t really agree with adding “headers” telling what type of values the different variables hold like GhostX4465 suggested. You can tell from the code that something is set to hold a number or a string, or that a variable is being declared (not initialized). IMO those extra comments and whitespace just makes it so you have to scroll further and can see less of the code on 1 screen. If that information is helpful to you, consider using type annotations instead. Although I definitely DO agree with grouping similar things together, like getting services and requiring modules at the very top of the script. The header thing isn’t a big deal, the most important thing is putting things in a sensible order, and picking a format and sticking to it.
Anyway, there’s also bugs in your code since you never disconnect the connections you make in tool.Equipped. If you equip then unequip, you can still rotate with R. Every time you equip again, it rotates a different amount because your InputBegan callback is called once for every connection that is made. Besides being incorrect, it will also slowly leak memory because the old connections never get cleaned up.
It’s a bit weird that the mouse events don’t also get connected several times. Apparenly they automatically get disconnected when the tool is unequipped, which is weird. But get in the habit of thinking about when your connections need to be disconnected.
You can fix your script like so:
local hammer = script.Parent
local ReplicatedStorage = game:GetService(“ReplicatedStorage”)
local cycleVal = 1
local chosenBuildable = “Floor”
local previewBuildable
local canBuild = nil
local isTouching = nil
local inputBeganConnection
hammer.Equipped:Connect(function(mouse)
viewBuildable()
mouse.Move:Connect(function()
mouse.TargetFilter = previewBuildable
previewBuildable.Position = mouse.Hit.Position
local Distance = (previewBuildable.Position - hammer.Handle.Position).Magnitude
if 25 < Distance then
canBuild = false
previewBuildable.Transparency = 1
else canBuild = true
previewBuildable.Transparency = 0
end
if canBuild == true then
previewBuildable.Color = Color3.new(0, 1, 0)
elseif canBuild == false then
previewBuildable.Color = Color3.new(1, 0, 0)
end
end)
mouse.Button1Down:Connect(function()
if canBuild == true then
ReplicatedStorage:WaitForChild("RemoteEvents"):WaitForChild("createBuildable"):FireServer(chosenBuildable, previewBuildable.Position, previewBuildable.Orientation)
end
end)
mouse.Button2Down:Connect(function()
cycleVal += 1
if cycleVal > 2 then
cycleVal = 1
end
if cycleVal == 1 then
chosenBuildable = "Floor"
elseif cycleVal == 2 then
chosenBuildable = "Wall"
end
viewBuildable()
end)
inputBeganConnection = game:GetService("UserInputService").InputBegan:Connect(function(input)
if input.KeyCode == Enum.KeyCode.R then
previewBuildable.Orientation += Vector3.new(0, 30, 0)
end
end)
end)
function viewBuildable()
if previewBuildable ~= nil then
previewBuildable:Destroy()
end
previewBuildable = ReplicatedStorage:WaitForChild(“Buildables”):FindFirstChild(chosenBuildable):Clone()
previewBuildable.Parent = workspace[“Short-Term”]
previewBuildable.Transparency = .5
previewBuildable.Material = “Plastic”
previewBuildable.CanCollide = false
previewBuildable.Position = game.Players.LocalPlayer:GetMouse().Hit.Position
end
thank you I didn’t notice this bug, also the mouse automatically gets disconnected when the tool is Unequipped because it is called from the Equipped function otherwise I think I would have to manually disconnect it aswell. Also I removed the unused “isTouching” variable