Feedback on my improved map voting script

local Players = game:GetService("Players")
local ServerStorage = game:GetService("ServerStorage")

local voting  = script.Parent
local padsFolder = voting.Pads 
local votingPadValues = ServerStorage.VotingPadValues

local function updateText(votingPad, voteText)
	voteText.Text = votingPadValues[votingPad.Name].Value
end

for _, votingPad in ipairs(padsFolder:GetChildren()) do
	
local voteText = votingPad.VoteScreen.Votes.SurfaceGui.Text
	
if votingPad ~= nil then
	
votingPad.Touched:Connect(function(hit)
	local player = Players:GetPlayerFromCharacter(hit.Parent)
		if player then
			local voted = ServerStorage[player.Name].Other:FindFirstChild("Voted")
				
			if voted.Value ~= votingPad then
			 votingPadValues[votingPad.Name].Value +=1
				
				if voted.Value ~= "N/A" then
					local previousPad = votingPadValues:FindFirstChild(voted.Value)
					votingPadValues[previousPad.Name].Value -= 1
				end
				voted.Value = votingPad.Name
			end
		end
	end)
	votingPadValues[votingPad.Name].Changed:Connect(function()
		    updateText(votingPad, voteText)
		end)
	end
end

Edits: The voting values of each pad are now stored inside a folder in ServerStorage and Player Voted Value as well, benefits in a bit better security and low memory.

1 Like

I think my main concern really is the fact you’ve used the Player itself to store values in place of doing it through the script itself.

You could easily do other methods such as storing it in a dictionary, with the pads either acting as ways to set the players choice (this would also enable you to detect accurately when a player leaves the proper tally number) or as vote platforms which tally.

Second MAJOR concern.
Logically speaking, every time Pad.Votes changes you create a new thread - that new thread then creates another three new connections as seen in the updateText, eventually this will slow down your game as the connections aren’t ever disconnected and they are all firing the same thing. By simply running updateText once you’d get the same implied logic as before.

1 Like

I personally think player would be better since there would be no use of dictionaries as it’s only 1 value. I’ve made changes to the script, thanks for letting me know.

Well in regards to data security and memory it prevents players changing their own data locally as well as also taking up less memory.

What is a better way of storing values in a script but not using dictionaries?

Just use arrays, the array will keep the data a bit more secure and help keep memory usages down. Creating the dictionary/array really isn’t an added level of complexity. Just go for it.

That’s my point here, what would I create the dictionary for?

For example, you’re storing the Voted.Value in Player.Data when you could be using an array/dictionary.

1 Like

I use client to server communication with RemoteEvent/RemoteFunctions to manage the vote counts in table, i put sanity check into my voting system to make sure. :stuck_out_tongue_winking_eye:

Not sure how it would be a security concern, if the player changes his value locally it would not affect the server at all. The only benefit is less memory, however one thing you have to do is manually remove the key of the player when he leaves the game, while storing an value inside the player would not require this since the player object is destoryed once he leaves

I’ve updated the script, the values are now stored in ServerStorage and the player’s cant change their values locally.

It depends on the system of input, technically any exploiting is a “security concern” - in this case its not much of a “concern” but more so a chance of data visualization being incorrect or things not being presented correctly if done client side (depends how he manipulates and uses it).

It would simply be a dictionary of player votes / information.

In a sense a player would then be submitting a vote much like in your system, except for you wouldn’t be using tags which is cleaner in a sense.

1 Like