How can i improve this dam function?

Help and Feedback Code Review
#1

I’ve been working on this script to add functionality to my dam as practice to get back into lua after several years. Is there a cleaner way for readability or more efficient way of doing this? Parts of the script i also took from a gate script i made several years back (CFrame for loops) so perhaps there’s a better way of doing that too.

local _items = {} 


local door = 1 
--				-1 = Closed
--				0 = Active
--				1 = Open

function getItems() 
	for _,v in pairs(script.Parent:GetChildren()) do 
		if ((v:IsA("BasePart")) and (v.Name == "Grate")) then 
			table.insert(_items,v) 
		end 
	end 
end 

function main() 
	local waterName = "Emitter"									
	local emitterName = "Water"									
	local model = script.Parent
	local d = door 
	door = 0 
	script.Parent.Button.Material = Enum.Material.Neon				
	script.Parent.NoisePart.Thrust:Play()    						-- plays sound of grates moving
	for i = 1,40 do 											
		for _,v in pairs(_items) do 
			v.CFrame = v.CFrame * CFrame.new(0,(0.20*(-d)),0) 		-- moves damn grates up and down
		end 
		wait() 
	end 
	if (d == (-1)) then 											-- turns on the water particles when grates are up
		for i, water in pairs(model:GetChildren())do		
			if water.Name == waterName then
			water[emitterName].Enabled = true			
			end
		end
		script.Parent.Button.Material = Enum.Material.Neon
	elseif (d == 1) then 											
		script.Parent.Button.Material = Enum.Material.SmoothPlastic
		for i, water in pairs(model:GetChildren())do				-- turns off the water particles when grates are down
			if water.Name == waterName then
			water[emitterName].Enabled = false					
			end
		end
	end 
	wait(.1)
	door = d 
	script.Parent.NoisePart.Thrust:Stop()						    -- stops moving sound
	script.Parent.NoisePart.Stop:Play()    							-- plays the 'stopping' sound
end 

script.Parent.Button.Click.MouseClick:connect(function() 			-- adds button click
	if (door == 0) then return end 
	door = (door*(-1)) 
	main() 
end) 

getItems()
#3

Efficieny wise there doesn’t seem much that you can do, although readabality wise you can probably do this.

local _items = {} 


local door = 1 
--				-1 = Closed
--				0 = Active
--				1 = Open

function getItems() 
	for _,v in pairs(script.Parent:GetChildren()) do 
		if ((v:IsA("BasePart")) and (v.Name == "Grate")) then 
			table.insert(_items,v) 
		end 
	end 
end 

function main() 
	local waterName = "Emitter"									
	local emitterName = "Water"									
	local model = script.Parent
	local d = door 
        local enabled
	door = 0 
	script.Parent.Button.Material = Enum.Material.Neon				
	script.Parent.NoisePart.Thrust:Play()    						-- plays sound of grates moving
	for i = 1,40 do 											
		for _,v in pairs(_items) do 
			v.CFrame = v.CFrame * CFrame.new(0,(0.20*(-d)),0) 		-- moves damn grates up and down
		end 
		wait() 
	end 
    if d == -1 then
        enabled = true
    elseif d == 1
        enabled = false
    end

	for i, water in pairs(model:GetChildren())do		
		if water.Name == waterName then
		water[emitterName].Enabled = enabled			
		end
	end
	script.Parent.Button.Material = Enum.Material.Neon

	wait(.1)
	door = d 
	script.Parent.NoisePart.Thrust:Stop()						    -- stops moving sound
	script.Parent.NoisePart.Stop:Play()    							-- plays the 'stopping' sound
end 

script.Parent.Button.Click.MouseClick:connect(function() 			-- adds button click
	if (door == 0) then return end 
	door = (door*(-1)) 
	main() 
end) 

getItems()
1 Like
#4

you can replace this line

door = (door*(-1))

with this

door = -door
3 Likes
#5

You don’t need to use parenthesis in if statements when you only have one condition being checked, however that is down to coding style.

I would recommend cleaning the code a bit and organizing it (spacing it where it should be spaced, and overall make it a bit cleaner.

You don’t need to use parenthesis with negative numbers. It is like you did in maths, but in Lua you can disregard that. :sunglasses:

You could do a biconditional check instead of having two loops and if statements, too. Here:

   	for i, water in pairs(model:GetChildren())do		
   		if water.Name == waterName then
   		water[emitterName].Enabled = (d == -1)
   		end
   	end

   	script.Parent.Button.Material = Enum.Material.Neon

Micro optimization, but can be useful if you ever reach gigantic amounts of elements as children of the models; you can use ipairs instead of pairs for instance children and descendants. This is a tiny bit faster.

2 Likes
#6

This is my code review:

local _items = {} 

local model = script.Parent
local button = model.Button
local noisePart = model.NoisePart

local waterName = "Emitter"									
local emitterName = "Water"									
	
local door = 1 
--			-1 = Closed
--			 0 = Active
--			 1 = Open

function getItems() 
	for _,v in pairs(model:GetChildren()) do 
		if v:IsA("BasePart") and v.Name == "Grate" then table.insert(_items,v) end 
	end 
end 

function main() 
	local d = door 
	door = 0
	button.Material = Enum.Material.Neon				
	noisePart.Thrust:Play()    										-- plays sound of grates moving
	for i = 1,40 do 											
		for _,v in pairs(_items) do 
			v.CFrame = v.CFrame * CFrame.new(0,-0.2*d,0) 		-- moves damn grates up and down
		end 
		wait() 
	end 
	
	if d == -1 then button.Material = Enum.Material.Neon end
	if d ==  1 then button.Material = Enum.Material.SmoothPlastic end
		
	for _, water in pairs(model:GetChildren())do		
		if water.Name == waterName then	water[emitterName].Enabled = d == -1 end
	end
	wait(.1)
	door = d 
	noisePart.Thrust:Stop()						    -- stops moving sound
	noisePart.Stop:Play()  							-- plays the 'stopping' sound
end 

button.Click.MouseClick:connect(function() 			-- adds button click
	if door ~= 0 then
		door = -door
		main()
	end
end) 

getItems()
1 Like