Is there a tidier way of writing this code?

Help and Feedback Scripting Support
1

I have this block of code which just checks how many players are in a list called “gamePlayers” and does some things based on how many players there are. I feel like there should be a more efficient way of doing this but I’m not sure how. If there is how could I go about doing it?

-- If the play button is clicked by any one of the players, a remote event is sent from the client and fires this function
playEvent.OnServerEvent:Connect(function()

	for i = #gamePlayers, 1, -1 do
		gamePlayers[i]:SetAttribute("InGame", true)
	end

	-- Checks if there is 6, 4 or 2 players

	if #gamePlayers == 6 then
		
		-- Sets the players spawn location just outside the arena instead of back in the lobby, so they can watch their teammates fight after they die
		
		for i = 3, 1, -1 do
			team1[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T1Spawn
			team2[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T2Spawn
		end
		
		-- Gets all of the players icons by requireing them from the module script that stored all the players icons when they joined the game
		
		local content1 = IconModule:GetIcon(gamePlayers[1])
		local content2 = IconModule:GetIcon(gamePlayers[2])
		local content3 = IconModule:GetIcon(gamePlayers[3])
		local content4 = IconModule:GetIcon(gamePlayers[4])
		local content5 = IconModule:GetIcon(gamePlayers[5])
		local content6 = IconModule:GetIcon(gamePlayers[6])
		
		for i = 6, 1, -1 do
			
			-- Disables the play button GUI
			-- Disables the menu GUI (Like the back to lobby button, inventory button and menu button)
			
			gamePlayers[i].PlayerGui.PlayButtonGui.Enabled = false
			gamePlayers[i].PlayerGui.Menu.Enabled = false
			
			-- Sets the GUI at the top of the screen to the teams wins
			
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team1Score.Text = team1Wins
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team2Score.Text = team2Wins
			
			-- Sets the player icons next to their teams score
			
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Image = content1
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player2.Image = content2
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player3.Image = content3
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Image = content4
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player2.Image = content5
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player3.Image = content6

			-- Sets the icon frames to visible (they still cant be seen by the players since the parent to these is still disabled)

			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player2.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player3.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player2.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player3.Visible = true
			
			-- Enables the screenGui so all of the above is now visible
			
			gamePlayers[i].PlayerGui.Scoreboard.Enabled = true
		end
		
		startEvent() -- Calls the main game function
	elseif #gamePlayers == 4 then -- The exact same as above but just for 4 people instead
		T1Player3Dead = true
		T2Player3Dead = true
		T1Player3Left = true
		T2Player3Left = true
		for i = 2, 1, -1 do
			team1[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T1Spawn
			team2[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T2Spawn
		end

		local content1 = IconModule:GetIcon(gamePlayers[1])
		local content2 = IconModule:GetIcon(gamePlayers[2])
		local content4 = IconModule:GetIcon(gamePlayers[3])
		local content5 = IconModule:GetIcon(gamePlayers[4])
		
		for i = 4, 1, -1 do

			gamePlayers[i].PlayerGui.PlayButtonGui.Enabled = false
			gamePlayers[i].PlayerGui.Menu.Enabled = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team1Score.Text = team1Wins
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team2Score.Text = team2Wins

			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Image = content1
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player2.Image = content2
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Image = content4
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player2.Image = content5

			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player2.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player3.Visible = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player2.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player3.Visible = false

			gamePlayers[i].PlayerGui.Scoreboard.Enabled = true

		end
		
		startEvent()
	elseif #gamePlayers == 2 then
		T1Player2Dead = true
		T1Player3Dead = true
		T2Player2Dead = true
		T2Player3Dead = true
		T1Player2Left = true
		T1Player3Left = true
		T2Player2Left = true
		T2Player3Left = true
		for i = 1, 1, -1 do
			team1[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T1Spawn
			team2[i].RespawnLocation = workspace:FindFirstChild("Sword Fighting Arenas").Arena1.T2Spawn
		end
		
		local content1 = IconModule:GetIcon(gamePlayers[1])
		local content4 = IconModule:GetIcon(gamePlayers[2])
		
		for i = 2, 1, -1 do
			
			gamePlayers[i].PlayerGui.PlayButtonGui.Enabled = false
			gamePlayers[i].PlayerGui.Menu.Enabled = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team1Score.Text = team1Wins
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Frame.Team2Score.Text = team2Wins

			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Image = content1
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Image = content4

			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player2.Visible = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team1.Player3.Visible = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player1.Visible = true
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player2.Visible = false
			gamePlayers[i].PlayerGui.Scoreboard.Frame.Team2.Player3.Visible = false

			gamePlayers[i].PlayerGui.Scoreboard.Enabled = true
			
		end
		
		startEvent()
	end
end)
1 Like
2

Please, PLEASE, just iterate over the table of players… otherwise your code will also error when the index goes out of range…

local players = game:GetService("Players")

for _, player in ipairs(players:GetPlayers()) do
    --do stuff
end

--or

for _, player in next, players:GetPlayers(), nil do
    --do stuff
end
1 Like
3

Thats what I would like to do but the issue is I have a gui for each of the players icons, and I only want it to be visible if there is actually a player, thats why I added the if statements, any ideas how I could do this in a for loop?

1 Like
4

make sure the number of players can only be 2, 4 or 6 else it will break lol. I also threw readability out of the window because your system is really unoptimized. btw idk how your game works as a whole so you may need to make amendments to get it working.

-- If the play button is clicked by any one of the players, a remote event is sent from the client and fires this function
Players = game:GetService("Players")
playEvent.OnServerEvent:Connect(function()
	--prepare your stuff
	local gamePlayers = Players:GetPlayers()
	local contents = {} --{TeamN_PlayerX = Icon}

	for _, plr in ipairs(gamePlayers) do
		plr:SetAttribute("InGame", true)
		contents["Team"..#contents < #gamePlayers/2 and "1" or "2".."_Player"..#gamePlayers/2 - #contents] = IconModule:GetIcon(plr)
	end

	T1Player2Dead = #gamePlayers == 2
	T1Player3Dead = #gamePlayers == 4
	T2Player2Dead = #gamePlayers == 2
	T2Player3Dead = #gamePlayers == 4
	T1Player2Left = #gamePlayers == 2
	T1Player3Left = #gamePlayers == 4
	T2Player2Left = #gamePlayers == 2
	T2Player3Left = #gamePlayers == 4

	for i = #gamePlayers / 2, 1, -1 do 
		--workspace["name with spaces"] is 8 times more efficient than findfirstchild
		team1[i].RespawnLocation = workspace["Sword Fighting Arenas"].Arena1.T1Spawn 
		team2[i].RespawnLocation = workspace["Sword Fighting Arenas"].Arena1.T2Spawn
	end

	for _, plr in ipairs(gamePlayers) do

		-- Disables the play button GUI
		-- Disables the menu GUI (Like the back to lobby button, inventory button and menu button)

		plr.PlayerGui.PlayButtonGui.Enabled = false
		plr.PlayerGui.Menu.Enabled = false

		-- Sets the GUI at the top of the screen to the teams wins

		plr.PlayerGui.Scoreboard.Frame.Frame.Team1Score.Text = team1Wins
		plr.PlayerGui.Scoreboard.Frame.Frame.Team2Score.Text = team2Wins

		-- Sets the player icons next to their teams score

		plr.PlayerGui.Scoreboard.Frame.Team1.Player1.Image = contents.Team1_Player1 or ""
		plr.PlayerGui.Scoreboard.Frame.Team1.Player2.Image = contents.Team1_Player2 or ""
		plr.PlayerGui.Scoreboard.Frame.Team1.Player3.Image = contents.Team1_Player3 or ""
		plr.PlayerGui.Scoreboard.Frame.Team2.Player1.Image = contents.Team2_Player1 or ""
		plr.PlayerGui.Scoreboard.Frame.Team2.Player2.Image = contents.Team2_Player2 or ""
		plr.PlayerGui.Scoreboard.Frame.Team2.Player3.Image = contents.Team2_Player3 or ""

		-- Sets the icon frames to visible (they still cant be seen by the players since the parent to these is still disabled)

		plr.PlayerGui.Scoreboard.Frame.Team1.Player1.Visible = true
		plr.PlayerGui.Scoreboard.Frame.Team1.Player2.Visible = #gamePlayers >= 4
		plr.PlayerGui.Scoreboard.Frame.Team1.Player3.Visible = #gamePlayers >= 6
		plr.PlayerGui.Scoreboard.Frame.Team2.Player1.Visible = true
		plr.PlayerGui.Scoreboard.Frame.Team2.Player2.Visible = #gamePlayers >= 4
		plr.PlayerGui.Scoreboard.Frame.Team2.Player3.Visible = #gamePlayers >= 6

		-- Enables the screenGui so all of the above is now visible

		plr.PlayerGui.Scoreboard.Enabled = true
	end

	startEvent() -- Calls the main game function
end)

lmk if you have any issues or the code has errors

2 Likes
5

Hard to make this more generic or function it out due to how it’s jumping around so much. It’s looks a bit over done but, really it’s pretty tight given how the Gui is set up for the players. To get this script to be more generic would have started with how the Gui is set up. This script is long due to that, working around the Gui set up. Sometimes you can’t help that when your Gui is so abstract (like a scoreboard). I have a few Scoreboards with scripts that look just like this.

2 Likes
6

I would use a GUI based on templates (eg. a player joins, they are added to the gui using a template frame) that is set up via code

1 Like