My Map Voting Script; Is it efficient and clean?

Help and Feedback Code Review
#1

Here’s my simple map voting script that I made, I’m all ears so I would like to see how can I improve my code.

local Players = game:GetService("Players")
	
local Voting  = script.Parent
local PadsFolder = Voting.Pads 

local function updateText(VotingPad)
	VotingPad.VoteScreen.Votes.SurfaceGui.Text.Text = VotingPad.Votes.Value
end

for _, VotingPad in ipairs(PadsFolder:GetChildren()) do
	
if VotingPad ~= nil then
	
VotingPad.Touched:Connect(function(hit)
	local Player = Players:GetPlayerFromCharacter(hit.Parent)
		if Player then
			local Voted = Player:WaitForChild("Data"):FindFirstChild("Other").Voted
				
			if Voted.Value ~= VotingPad then
				VotingPad.Votes.Value += 1
				
				if Voted.Value ~= "N/A" then
					local PreviousPad = PadsFolder:FindFirstChild(Voted.Value)
					PreviousPad.Votes.Value -= 1
				end
				Voted.Value = VotingPad.Name
			end
		end
	end)
	VotingPad.Votes.Changed:Connect(function()
		    updateText(VotingPad)
		end)
	end
end
5 Likes
#2

You could wrap the three touched event into a for loop and also, instead of making 3 functions to update the gui make only one.

2 Likes
#3

You’re updating something in a loop. Almost always this can be replaced with an event listener instead. Maybe the Changed for your value object?

1 Like
#4

Oh yeah, I forgot. I’ll use :GetPropertyChangedSignal.

#5

You can make some minor changes that will reduce the amount of code you’re using, by reusing the same code DRY Principle (Don’t repeat yourself)

local Players = game:GetService("Players")
	
local VotingSystem  = script.Parent
	
local pads = {
	VotingSystem.Pad1,
	VotingSystem.Pad2,
	VotingSystem.Pad3
}

local voteScreens = {
	VotingSystem.VoteScreen1,
	VotingSystem.VoteScreen2,
	VotingSystem.VoteScreen3
}

local function updateVoteScreenDisplay(index)
	voteScreens[index].Votes.SurfaceGui.Text.Text = tostring(pads[index].Votes.Value) .. " Votes"
end
	
for i, pad in ipairs(pads) do
	pad.Touched:Connect(function(hit)
		local Player = Players:GetPlayerFromCharacter(hit.Parent)
		if Player then
			if Player.Voted.Value ~= pad then
				pad.Votes.Value = pad.Votes.Value + 1
				
				if Player.Voted.Value ~= "N/A" then
					local PreviousPadValue = VotingSystem:FindFirstChild(Player.Voted.Value)
					PreviousPadValue.Votes.Value = PreviousPadValue.Votes.Value - 1
				end
				Player.Voted.Value = pad.Name
				updateVoteScreenDisplay(index)
			end
		end
	end)
end

I wrote this pretty quick so there could be a typo or two

2 Likes
#6

This is not that flexible. If you want to add say a Pad4 for example, not saying you want to or will add one but let’s say for the sake of good practice, You have to add a Pad4 declaration and a Pad4 touched event AND a votes changed value. Now, I’m not one to use Instance values anymore in my code but I see why people do it. I would suggest looping through all the pads. Adding them all to a folder and looping through that folder. This way you can event wrap the Votes.Changed() inside of that loop and if you want to add a 4th vote for example, you could just add another pad into the folder.

It’s important to note that it’s good practice to make your code as flexible as possible. Even if you don’t want a 4th vote, opening that possibility could really go a long ways with your development in both practice and future systems you may want to add.

5 Likes
#7

Hello, I would reccomond using tables do easily add pads on one go, like my voting system here: