How could I make this more compact?

Help and Feedback Code Review
#1

I’m working on a building system, everything works really well but It’s hard to look at it and make changes. Do you know how to make It more understandable and more compact and easier to read?

local uis = game:GetService("UserInputService")
local towerplace = nil
local towers = game.ReplicatedStorage.Towers
local gui = script.Parent
local rotate = false
local placeable = true
local runservice = game:GetService('RunService')
local function raycast(blacklist)
	local mouse = uis:GetMouseLocation()
	local raycast1 = camera:ViewportPointToRay(mouse.X, mouse.Y)
	local params = RaycastParams.new()
	params.FilterType = Enum.RaycastFilterType.Blacklist
	local ovParams = OverlapParams.new() --this creates a blank OverlapParams thing
	ovParams.FilterDescendantsInstances = {}
	ovParams.CollisionGroup = "Default"
	ovParams.FilterType = Enum.RaycastFilterType.Whitelist
	ovParams.MaxParts = 1
	params.FilterDescendantsInstances = {towerplace}

	local raycast2 = workspace:Raycast(raycast1.Origin, raycast1.Direction * 50, params, ovParams)
	return raycast2
	
end

script.Parent.MouseEnter:Connect(function()
	placeable = false
end)
script.Parent.MouseLeave:Connect(function()
	placeable = true
end)
uis.InputBegan:Connect(function(int) --Int, the keycode

	if int.UserInputType == Enum.UserInputType.MouseButton1 then --Checks if player pressed Click we did that cause we used UserInputService
		if towerplace then
			if towerplace.BrickColor == BrickColor.new("Really red") then
			else
				if placeable then
		local pos = towerplace.Position
			local name = towerplace.Name
			local orientation = towerplace.Orientation
			
			game.ReplicatedStorage.Events.Place:FireServer(pos, name, orientation)
			towerplace:Destroy()
			towerplace = nil
			end
		end	
		end
		end
end)

uis.InputBegan:Connect(function(int)
	if int.KeyCode == Enum.KeyCode.R then
		if placeable then
		towerplace.Orientation = Vector3.new(towerplace.Orientation.X, towerplace.Orientation.Y + 10, towerplace.Orientation.Z)
		end
	end

end)
uis.InputBegan:Connect(function(int)
	if int.KeyCode == Enum.KeyCode.T then
		if placeable then
		towerplace.Orientation = Vector3.new(towerplace.Orientation.X, towerplace.Orientation.Y, towerplace.Orientation.Z + 10)
end
	end

end)
uis.InputBegan:Connect(function(int)
	if int.KeyCode == Enum.KeyCode.Y then
		if placeable then
		towerplace.Orientation = Vector3.new(towerplace.Orientation.X, towerplace.Orientation.Y - 10, towerplace.Orientation.Z)
	end
end
end)
uis.InputBegan:Connect(function(int)
	if int.KeyCode == Enum.KeyCode.X then
		if placeable then
		towerplace.Orientation = Vector3.new(0, 0, 0)
	end
end
end)
local function settower(name)
	local towerexist = towers:FindFirstChild(name)
	if towerexist then
		if towerplace then
			towerplace:Destroy()
		end
			
			towerplace = towerexist:Clone()
		towerplace.Parent = workspace
		end
end
for i, v in pairs(gui:GetChildren()) do
	if v:IsA("TextButton") then
		if towerplace then
	v.MouseEnter:Connect(function()
		placeable = false
	end)
	v.MouseLeave:Connect(function()
		placeable = true
			end)
			end
		end
end
--START


gui.wall.MouseButton1Click:Connect(function()
	settower("WoodWall")
end)
gui.Phone.MouseButton1Click:Connect(function()
	settower("Phone")
end)
gui.chair.MouseButton1Click:Connect(function()
	settower("Chair")
end)
gui.Stand.MouseButton1Click:Connect(function()
	settower("Stand")
end)
gui.Dog.MouseButton1Click:Connect(function()
	settower("HotDog")
end)
gui.Music.MouseButton1Click:Connect(function()
	settower("Music")
end)
gui.Hat1.MouseButton1Click:Connect(function()
	settower("Hat1")
end)
gui.Glass.MouseButton1Click:Connect(function()
	settower("Glass")
end)
gui.LongGlass.MouseButton1Click:Connect(function()
	settower("2Xglass")
end)
gui.Glazz.MouseButton1Click:Connect(function()
	settower("4X4Glass")
end)
gui.onexonelader.MouseButton1Click:Connect(function()
	settower("1x1ladder")
end)
gui.onexfivelad.MouseButton1Click:Connect(function()
	settower("1x5ladder")
end)
gui.Door.MouseButton1Click:Connect(function()
	settower("Door")
end)
gui.tv.MouseButton1Click:Connect(function()
	settower("FlatScreen")
end)
gui.IOS.MouseButton1Click:Connect(function()
	settower("Iphone")
end)
gui.saul.MouseButton1Click:Connect(function()
	settower("Saul")
end)
gui.Fenceone.MouseButton1Click:Connect(function()
	settower("Fence1x1")
end)
gui.Barrier.MouseButton1Click:Connect(function()
	settower("Barrier")
end)
gui.Error.MouseButton1Click:Connect(function()
	settower("Error")
end)
gui.Barrel.MouseButton1Click:Connect(function()
	settower("Barrel")
end)
gui.Radio.MouseButton1Click:Connect(function()
	settower("radio")
end)
--END











while true do
	task.wait()
	if towerplace then
		local result = raycast()
		
		if result and result.Instance then
			
			local x = math.floor(result.Position.X)
			local y = math.floor(result.Position.Y + towerplace.Size.Y / 2)
			local z = math.floor(result.Position.Z)
			if towerplace.Orientation == Vector3.new(towerplace.Orientation.X, towerplace.Orientation.Y, 90)  then
				y = result.Position.Y
			else
				y = result.Position.Y + towerplace.Size.Y/ 2
			
end
			
			if result.Instance.Name == "Snap" then
				towerplace.Position = result.Instance.Position
				towerplace.Orientation = result.Instance.Orientation
			else
				towerplace.Position = Vector3.new(x,y,z)
				
			end
			towerplace.Transparency = 0.2
			towerplace.Material = Enum.Material.ForceField
	local touching = towerplace:GetTouchingParts()
			if result.Instance.Name == "Notouch" then
				towerplace.BrickColor = BrickColor.new("Really red")
				else
				towerplace.BrickColor = BrickColor.new("Lime green")

	end
		end
	end
end




--First script
game.ReplicatedStorage.Events.Place.OnServerEvent:Connect(function(plr, pos, name, orientation)
	local newpart = game.ReplicatedStorage.Towers:FindFirstChild(name):Clone()
	newpart.Parent = workspace.Builds[plr.UserId]
	newpart.Position = pos
	newpart.Orientation = orientation
	newpart.Name = plr.Name
	newpart.CanCollide = true
	newpart.CanTouch = true
	newpart.CanQuery = true
	newpart.CastShadow = true
end)

--Second script

game.Players.PlayerAdded:Connect(function(plr)
	local folder = Instance.new("Folder")
	folder.Name = plr.UserId
	folder.Parent = workspace.Builds
end)

game.Players.PlayerRemoving:Connect(function(plr)
	for i,v in pairs(game.Workspace.Builds:GetChildren()) do
		if v.Name == plr.UserId then
			v:Destroy()
		end
	end
end)

--All these are seperate scripts

It’s not hard to add items but it would be better to make changes easier.
You see the notes there? I added them to make it easier to read but it barely helps.
If you know how to make It more compact and easier to read tell me.

#2

There are way too many uis.BeganInput connections when you can handle them all at once, with if ... elseif ... else blocks.

Also, the massive amount of gui.GUIITEM.MouseButton1Click can be simplified into key-pair values in a table, where the key is the GUI item, and the value is the tower name. Then, just loop with for k, v in table do and make the connections with k.MouseButton1Click:Connect().

If you don't mind, I have made revisions. Open this if you want to see. This takes a huge part of my post.

Placement script

Changes:

  • All uis.BeganInput connections now merged into one.
  • Moved code from raycast() that creates a new RaycastParams and OverlapParams and their property changes outside of the function mentioned a sentence ago. Setting the FilterDescendantsInstances remains in the function.
  • Further compressed the connections in the last part of the code to a key-value pair ([UI_Object] = “TowerName”), with a for-loop block to connect all of them.
  • Simplified tower rotation with Vector3.yAxis, Vector3.zAxis, Vector3.zero, and assignment operators (+=, -=).
  • Switched from while true do to runservice.RenderStepped:Connect() al the bottommost part of the Script.
  • Wrapped most of the connection logic into a do-block. This allows that part of the code to be collapsible (in studio).
  • Reordered large parts of the code, with event handling going last.

Revisioned code:

Long code. Click to show 
local uis = game:GetService("UserInputService")
local towerplace = nil
local towers = game.ReplicatedStorage.Towers
local gui = script.Parent
local rotate = false
local placeable = true
local runservice = game:GetService('RunService')
local camera = workspace.CurrentCamera

local params = RaycastParams.new() -- this is reuseable
params.FilterType = Enum.RaycastFilterType.Blacklist

local ovParams = OverlapParams.new() --this creates a blank OverlapParams thing. this is reuseable
ovParams.CollisionGroup = "Default"
ovParams.FilterType = Enum.RaycastFilterType.Whitelist
ovParams.MaxParts = 1

local function raycast(blacklist)
	local mouse = uis:GetMouseLocation()
	local raycast1 = camera:ViewportPointToRay(mouse.X, mouse.Y)
	
	params.FilterDescendantsInstances = {towerplace} -- towerplace may change.

	return workspace:Raycast(raycast1.Origin, raycast1.Direction * 50, params, ovParams)
end

local function settower(name)
	local towerexist = towers:FindFirstChild(name)
	if towerexist then
		if towerplace then
			towerplace:Destroy()
		end

		towerplace = towerexist:Clone()
		towerplace.Parent = workspace
	end
end

-- main placement program
runservice.RenderStepped:Connect(function() -- connect this function to renderstepped.
	if towerplace then
		local result = raycast()

		if result and result.Instance then

			local x = math.floor(result.Position.X)
			local y = math.floor(result.Position.Y + towerplace.Size.Y / 2)
			local z = math.floor(result.Position.Z)
			
			if towerplace.Orientation == Vector3.new(towerplace.Orientation.X, towerplace.Orientation.Y, 90)  then
				y = result.Position.Y
			else
				y = result.Position.Y + towerplace.Size.Y/ 2
			end

			if result.Instance.Name == "Snap" then
				towerplace.Position = result.Instance.Position
				towerplace.Orientation = result.Instance.Orientation
			else
				towerplace.Position = Vector3.new(x,y,z)
			end
			
			towerplace.Transparency = 0.2
			towerplace.Material = Enum.Material.ForceField
			
			--local touching = towerplace:GetTouchingParts() -- not yet used. commented out for now.
			
			if result.Instance.Name == "Notouch" then
				towerplace.BrickColor = BrickColor.new("Really red")
			else
				towerplace.BrickColor = BrickColor.new("Lime green")
			end
		end
	end
end)

-- start creating connections. collapsible using do-end block.
local connections = {}

do
	local elements = { -- pair the ui element and the name of the tower.
		[gui.wall] = "WoodWall", -- almost anything can be a key.
		[gui.Phone] = "Phone",
		[gui.chair] = "Chair",
		[gui.Stand] = "Stand",
		[gui.Dog] = "HotDog",
		[gui.Music] = "Music",
		[gui.Hat1] = "Hat1",
		[gui.Glass] = "Glass",
		[gui.LongGlass] = "2Xglass",
		[gui.Glazz] = "4X4glass",
		[gui.onexonelader] = "1x1ladder",
		[gui.onexfivelad] = "1x5ladder",
		[gui.Door] = "Door",
		[gui.tv] = "FlatScreen",
		[gui.IOS] = "Iphone",
		[gui.saul] = "Saul",
		[gui.Fenceone] = "Fence1x1",
		[gui.Barrier] = "Barrier",
		[gui.Barrel] = "Barrel",
		[gui.Radio] = "radio"
	}

	-- save the connections if needed later.
	for button, towername in elements do -- iterate through all key-value pairs. connect settower(towername) to each ui button.
		connections[towername] = button.MouseButton1Click:Connect(function()
			settower(towername)
		end)
	end

	-- user input here
	uis.InputBegan:Connect(function(int)
		if not placeable then
			return -- regardless of key press, if towerplace is not placeable, do not test input
		end

		if int.UserInputType == Enum.UserInputType.MouseButton1 then -- we need to know if the user can place a tower.
			if towerplace then
				if not (towerplace.BrickColor == BrickColor.new("Really red")) then -- can the user place the tower?
					local pos = towerplace.Position
					local name = towerplace.Name
					local orientation = towerplace.Orientation

					game.ReplicatedStorage.Events.Place:FireServer(pos, name, orientation) -- we can place the tower, let's inform the server.
					towerplace:Destroy()
					towerplace = nil
				end	
			end
		elseif int.UserInputType == Enum.UserInputType.Keyboard then -- capture key press.
			local key = int.KeyCode

			if key == Enum.KeyCode.X then -- reset rotation
				towerplace.Orientation = Vector3.zero -- returns a zero-filled vector3. also possible in vector2
			elseif key == Enum.KeyCode.Y then -- rotate sideways
				towerplace.Orientation -= Vector3.yAxis * 10 -- assignment operator. -= assigns variable to itself, and subtracts to the next value.
			elseif key == Enum.KeyCode.R then -- rotate opposite direction
				towerplace.Orientation += Vector3.yAxis * 10
			elseif key == Enum.KeyCode.T then -- tilt the tower?
				towerplace.Orientation += Vector3.zAxis * 10
			end
		end
	end)

	-- loop through all buttons. make them change placeable.
	for _, v in gui:GetChildren() do -- you can just slap in the table without ipairs or pairs.
		if v:IsA("TextButton") then
			if towerplace then
				v.MouseEnter:Connect(function()
					placeable = false
				end)
				v.MouseLeave:Connect(function()
					placeable = true
				end)
			end
		end
	end

	-- do the same for the parent button here.
	script.Parent.MouseEnter:Connect(function()
		placeable = false
	end)
	script.Parent.MouseLeave:Connect(function()
		placeable = true
	end)
end

Server "OnPlace" script

Changes:

  • There were not many changes.
  • Added local repstore = game:GetService("ReplicatedStorage"). GetService() allows you to get a service even if its name has been changed.
  • Replaced game.ReplicatedStorage.Events.Place to local placeevent = repstore.Events.Place.
  • Added a suggestion to change properties of the towers manually instead. Remove the comments if you still prefer old behavior.

Revisioned code:

local repstore = game:GetService("ReplicatedStorage")
local placeevent = repstore.Events.Place

placeevent.OnServerEvent:Connect(function(plr, pos, name, orientation)
	local newpart = repstore.Towers:FindFirstChild(name):Clone()
	newpart.Parent = workspace.Builds[plr.UserId]
	newpart.Position = pos
	newpart.Orientation = orientation
	newpart.Name = plr.Name
	-- these next few lines can be set manually in the explorer.
	--newpart.CanCollide = true
	--newpart.CanTouch = true
	--newpart.CanQuery = true
	--newpart.CastShadow = true
end)

Server Players script

Changes:

  • There were not many changes.
  • Instance.new allows you to add a second parameter, which sets the new item’s parent to where you want it to.
  • Added local plrs = game:GetService() to shorted connection code.
  • Added a break in the PlayerRemoving loop. When the build is found, the loop stops.

Revisioned code:

local plrs = game:GetService("Players")

plrs.PlayerAdded:Connect(function(plr)
	local folder = Instance.new("Folder", workspace.Builds) -- create the instance, parent it afterwards.
	folder.Name = plr.UserId
end)

plrs.PlayerRemoving:Connect(function(plr)
	for _, v in workspace.Builds:GetChildren() do
		if v.Name == plr.UserId then
			v:Destroy()
			break -- stop the loop once the leaving player's builds folder is found.
		end
	end
end)

Let me know if something does not work.

1 Like
Building system simple to add items
#3

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.