How could i improve my code,

Help and Feedback Code Review
#1

hello guys i’m kinda a new dev and a made a vote system but i dont really like it it work fine but i feel i can make it shorter and faster & better, so it is a two part local script and ServerScriptService script

--ServerScriptService 

local map
local ChooseMapsisdone = game.ReplicatedStorage:WaitForChild("chooseMaps")
local updatedvote = game.ReplicatedStorage:WaitForChild("updateVote")
local AddRemoveVote = game.ReplicatedStorage:WaitForChild('AddRemoveVote')
--------------------------------------------------------------------------------
local MapsFolder = game.ReplicatedStorage:WaitForChild("Maps")
local Players = game.Players
local StarterGui = game.StarterGui
local VoteGui = StarterGui:WaitForChild("VoteGui")
local SelcetedMaps = {}
local MapsVotes = {0,0,0}
local PlayerVoted = {}
local playersAlive = {}
local roundstarted = false
local sword = game.ReplicatedStorage:WaitForChild("ClassicSword",3)

local function FindTheMax(Table)
	local biggestValue =  Table[1]

	for _,value in Table do
		if value > biggestValue then
			biggestValue = value
		end
	end
	return biggestValue
end
local function ChooseMaps()
	local Maps = MapsFolder:GetChildren()
	for i,Map in Maps do
		local randomMap = Maps[math.random(1,#Maps)]
		if table.find(SelcetedMaps,randomMap) ~= nil then -- table.find return nil if value is not there 
			repeat
				randomMap = Maps[math.random(1,#Maps)]
			until table.find(SelcetedMaps,randomMap) == nil -- this mean the map that have been choosen is not already on the SelcetedMaps
		end
		table.insert(SelcetedMaps,randomMap)

		if #SelcetedMaps == 3 then
			break
		end
	end
end
local function MovePlayers()
	for _,player in Players:GetChildren() do
		print(player)
		table.insert(playersAlive,player)
		local hu:Humanoid = player.character:FindFirstChild("Humanoid")
		hu.StateChanged:Connect(function(old,new)
			if new == Enum.HumanoidStateType.Dead then
				local findPlayer = table.find(playersAlive,player)
				if findPlayer  then
					print(findPlayer)
					table.remove(playersAlive,findPlayer)
					print(playersAlive)
				end
			end
		end)
		local clonesword = sword:Clone()
		clonesword.Parent = player.Backpack
		for _,Stuff in map:GetChildren() do
			local SpawnFolder = map:WaitForChild("SpwanLocation"):GetChildren()
			local humanoidRootPart  = player.character:WaitForChild("HumanoidRootPart")
			for __,spwan in SpawnFolder   do
				local randomSpwaner = SpawnFolder[math.random(1,#SpawnFolder)]
				humanoidRootPart.CFrame = randomSpwaner.CFrame
				break
			end
		end
	end
end
local function GameRound()

	ChooseMaps()
	----events------

	Players.PlayerAdded:Connect(function(player)
		updatedvote:FireClient(player,nil,nil,true)
		ChooseMapsisdone:FireClient(player,SelcetedMaps)

	end)
	for _,player in Players:GetChildren() do
		updatedvote:FireClient(player,nil,nil,true)
		ChooseMapsisdone:FireClient(player,SelcetedMaps)
		--[[local voteGui = player.PlayerGui:WaitForChild("VoteGui")
		local MineFrame = voteGui.MineFrame
		for Index,Frame in MineFrame:GetChildren() do
			local MapClone =SelcetedMaps[Index]:Clone()
			MapClone.Parent = 	Frame.MapImage.ViewportFrame 
			Frame.MapImage.ViewportFrame.CurrentCamera = MapClone:WaitForChild("")

		end]]


	end
	--[[VoteEvent.OnServerEvent:Connect(function(player,Index)
		if table.find(PlayerVoted,player) == nil then
			print("XXX")
			MapsVotes[Index] += 1
			updatedvote:FireAllClients(Index,MapsVotes[Index])
			table.insert(PlayerVoted,player)

		end

	end)]]

	local VoteSystem = AddRemoveVote.OnServerEvent:Connect(function(p,PlayerVoteFor,Index)
		if PlayerVoteFor == Index then
			return
		end
		if PlayerVoteFor > 0 then
			print(PlayerVoteFor,Index) -- old vote is playervotefor new is index
			MapsVotes[PlayerVoteFor] -= 1
			updatedvote:FireAllClients(PlayerVoteFor,MapsVotes[PlayerVoteFor])

		end
		MapsVotes[Index] += 1
		updatedvote:FireAllClients(Index,MapsVotes[Index])

	end)


	--- move players after chooseing a map

	task.wait(8)
	VoteSystem:Disconnect()
	updatedvote:FireAllClients(nil,nil,false)

	local x = FindTheMax(MapsVotes)
	map = SelcetedMaps[table.find(MapsVotes,x)]:Clone()
	map.Parent = workspace
	workspace:WaitForChild(map,3)
	print(map)

	MovePlayers()

end
GameRound()
local function Clear()
	table.clear(playersAlive)
	table.clear(SelcetedMaps)
	table.clone(PlayerVoted)
	for i,value in MapsVotes do
		MapsVotes[i]= 0
	end
	print(map)
	map:Destroy()
	updatedvote:FireAllClients(nil,nil,nil,true)
	print(playersAlive,SelcetedMaps,PlayerVoted,MapsVotes)
	print("Cleared")
	GameRound()

end
while task.wait(1.5) do
	if #playersAlive == 0 then
		Clear()
		continue
	end
end

local script

local ChooseingMap = game.ReplicatedStorage:WaitForChild("chooseMaps")
local MineFrame = script.Parent.MineFrame
local updateVote = game.ReplicatedStorage:WaitForChild("updateVote")
local LocalPlayer =  game:GetService("Players").LocalPlayer
local AddRemoveVote = game.ReplicatedStorage:WaitForChild("AddRemoveVote")
local db = false
local PlayerVoteFor = 0


for Index,MapFrame in MineFrame:GetChildren() do
	local voteButton :TextButton = MapFrame.VoteButton
	voteButton.MouseButton1Click:Connect(function()
            --[[
                        
            if PlayerVoteFor == Index then
                VoteEvent:FireServer(Index)
                
            else]]
			--=print(MineFrame,PlayerVoteFor,Index)
			-- X print(PlayerVoteFor,Index)
			-- X AddRemoveVote:FireServer(PlayerVoteFor,Index)
			--X PlayerVoteFor = Index



			--end
			AddRemoveVote:FireServer(PlayerVoteFor,Index)
			PlayerVoteFor = Index
		end)
	if Index == 3 then
		break
	end
end
ChooseingMap.OnClientEvent:Connect(function(selectedMaps)
	print(selectedMaps)
	for Index,MapFrame in MineFrame:GetChildren() do
		local currentMap = selectedMaps[Index]
		print(MapFrame)
		MapFrame:WaitForChild("MapName",2).Text = tostring(currentMap)
		local  MapImage = MapFrame.MapImage
		local MapCloneToImageFrame : Model = currentMap:Clone()
		local CamreaTo = MapCloneToImageFrame:FindFirstChild("CameraHere")
		local viewPort  = MapImage.ViewportFrame
		local Camera = Instance.new("Camera")
		Camera.Parent = viewPort
		Camera.CFrame = CamreaTo.CFrame
		viewPort.CurrentCamera = Camera
		MapCloneToImageFrame.Parent = viewPort																																					

		if Index == 3 then
			break
		end
	end
end)
updateVote.OnClientEvent:Connect(function(Index,voteCount,OpenGui,clear)
	--    print(OpenGui)
	if OpenGui == true  then
		LocalPlayer.PlayerGui.VoteGui.Enabled = true
		--    print(OpenGui)

	elseif OpenGui == false then

		LocalPlayer.PlayerGui.VoteGui.Enabled = false
	end
	if Index and voteCount then
		LocalPlayer.PlayerGui.VoteGui.MineFrame:GetChildren()[Index]:WaitForChild("Votes").Text = voteCount 
	end
	if clear == true then
		for _,frame in LocalPlayer.PlayerGui.VoteGui.MineFrame:GetChildren() do
			frame.Votes.Text = 0
			frame.MapImage.ViewportFrame:FindFirstChildOfClass("Model"):Destroy()
		end
		PlayerVoteFor = 0
		
	end
	--end
	--script.Parent.Parent.Enabled = updatevotegui
end)

i feel like my code written by an alien :slight_smile:

#2

using the enter bar whenever changing code. example

local replicatedStorage = game:GetService("ReplicatedStorage")
local players = game:GetService("Players")

local testRemote = replicatedStorage.Test

local test = math.random(1, 2)

local function ChangeTest(max, min)
    test = math.random(min, max)
end

testRemote.OnServerEvent:Connect(function(player, ...)
    local args = {...}
    print(player.DisplayName.." has "..args.." to say")
end)

players.PlayerAdded:Connect(function(player)
    print(player.DisplayName)
end)

this makes it a bit more organized, putting the service functions at the bottom, local functions or script functions in the middle, service variables are first, then instance variables are next.

making other devs be able to know what your code is saying

2 Likes