What could be improved here?

Help and Feedback Code Review
, ,
#1

Provide an overview of:

  • What does the code do and what are you not satisfied with?
    It’s a building system, with a size scaler and a remove parts button. Im not satisfied that it looks unreadable and could use some improvement.
  • What potential improvements have you considered?
    Format it better with comments and functions.
  • How (specifically) do you want to improve the code?
    Put functions code in functions for readability.
local Player = game.Players.LocalPlayer
local Mouse = Player:GetMouse()

local Size = {
	["X"] = 4,
	["Y"] = 1,
	["Z"] = 2,
}

script.Parent.Size.Parent = Player.PlayerGui

script.Parent.Equipped:Connect(function(mouse)
	print("Equipped")
	Player.PlayerGui.Size.Enabled = true
	
	local PartPlacer = Instance.new("Part")
	PartPlacer.Parent = workspace
	PartPlacer.Anchored = true
	PartPlacer.Transparency = 0.5
	PartPlacer.CanCollide = false
	PartPlacer.Name = "PartPlacer"
	
	PartPlacer.Size = Vector3.new(Size.X,Size.Y,Size.Z)
	
	local Folder = Instance.new("Folder")
	Folder.Name = "Parts"
	Folder.Parent = workspace
	
	Mouse.TargetFilter = PartPlacer

	local PartPlacerpos = game["Run Service"].RenderStepped:Connect(function()
		PartPlacer.CFrame = CFrame.new(Mouse.Hit.Position)+Vector3.new(0,0.5,0)
	end)
	
	Player.PlayerGui.Size.X.FocusLost:Connect(function(enterpressed)
		local Text = Player.PlayerGui.Size.X.Text
		local XNum = tonumber(Text)
		print(XNum)
		if XNum then
			PartPlacer.Size = Vector3.new(XNum,PartPlacer.Size.Y,PartPlacer.Size.Z)
			Size.X = XNum
		else
			print("Not a number")
		end
	end)
	
	Player.PlayerGui.Size.Y.FocusLost:Connect(function(enterpressed)
		local Text = Player.PlayerGui.Size.Y.Text
		local YNum = tonumber(Text)
		print(YNum)
		if YNum then
			PartPlacer.Size = Vector3.new(PartPlacer.Size.X,YNum,PartPlacer.Size.Z)
			Size.Y = YNum
		else
			print("Not a number")
		end
	end)
	
	Player.PlayerGui.Size.Z.FocusLost:Connect(function(enterpressed)
		local Text = Player.PlayerGui.Size.Z.Text
		local ZNum = tonumber(Text)
		print(ZNum)
		if ZNum then
			PartPlacer.Size = Vector3.new(PartPlacer.Size.X,PartPlacer.Size.Y,ZNum)
			Size.Z = ZNum
		else
			print("Not a number")
		end
	end)
	
	script.Parent.Activated:Connect(function()
		local PlacedPart = Instance.new("Part")
		PlacedPart.Name = "Part"
		PlacedPart.Parent = Folder
		PlacedPart.Anchored = true
		PlacedPart.Position = PartPlacer.Position
		PlacedPart.Size = PartPlacer.Size
		script.Parent.Click:Play()
	end)
	
	script.Parent.Unequipped:Connect(function()
		Player.PlayerGui.Size.Enabled = false
		PartPlacer:Destroy()
	end)
	
	Player.PlayerGui.Size.ClearParts.MouseButton1Click:Connect(function()
		for i,v in pairs(Folder:GetChildren()) do
			v:Destroy()
			script.Parent.Click:Play()
		end
	end)
end)
#2

Instead of wrapping everything into one single Equipped:Connect function, I would break everything down into separate functions. For instance, you have a function that connects the PlayerGui.Y.FocusLost. I would create a separate function for that called onFocusLost() and call it within your script.Parent.Equipped:Connect() function.

Also it’s pretty outdated to have scripts as children of a tool since it uses more memory if there are multiple tools with the same exact script. It’s quite redundant. Instead, I would create a single module that clones the tool and has all the functions in one script.

#3

I have a habit of doing that. Thank you!