Looking for feedback on a custom Leaderboard

[Capyb_ra]

Hey, I made this custom leaderboard for a game I’m working on with a friend, I’m looking for feedback on what you guys think as well as how I could improve it.

A few questions I have are

1. Have I handled connections correctly to prevent memory leaks?
2. Is there a better way to toggle the leaderboard ie. not having to use an if statement to check which tween to run.
3. Could I possibly optimize it any more?

-- NotBroham
-- // Variables \\ --
local uis = game:GetService("UserInputService")
local plr = game.Players.LocalPlayer
local template = script:WaitForChild('TEMPLATE') -- // The frame's template
local thumbnailurl = "https://www.roblox.com/bust-thumbnail/image?userId=%s&width=420&height=420&format=png" -- // Replace %s with a userid to get their avatar thumbnail
local open = false
local KeyCode = Enum.KeyCode.Tab

local connections = {} -- // Table so we can disconnect connections when we're done with them

-- // Functions/Loops \\ -- 
local function newPlayer(player) -- // Add a new player to the leaderboard/handle connections
	local leaderstats = player:WaitForChild('leaderstats')
	local cl = template:Clone()
	cl.Name = player.Name
	cl.Parent = script.Parent
	cl.TextLabel.Text = leaderstats:WaitForChild('Kills').Value
	local id = game.Players:GetUserIdFromNameAsync(player.Name)
	cl.ImageLabel.Image = string.format(thumbnailurl,id)
	
	connections[player] = player:WaitForChild('leaderstats'):WaitForChild('Kills').Changed:Connect(function(new)
		cl.LayoutOrder = -new -- // Order the leaderboard from most to least kills
		cl.TextLabel.Text = new
	end)
end

local function toggleLeaderboard() -- // Open/Close the leaderboard
	open = not open
	if open then -- // Wondering if I could do this without having to use a variable
		script.Parent:TweenPosition(UDim2.new(0.289, 0,0, 0),Enum.EasingDirection.Out,Enum.EasingStyle.Linear,.5,true)
	else
		script.Parent:TweenPosition(UDim2.new(0.289, 0,-1, 0),Enum.EasingDirection.Out,Enum.EasingStyle.Linear,.5,true)
	end
end

for i,v in next, game.Players:GetPlayers() do -- // Loop for the players who joined before us/ourself
	newPlayer(v)
end

game.Players.PlayerAdded:Connect(function(player) -- // I could do game.Players.PlayerAdded:Connect(newPlayer) but I think this looks better
	newPlayer(player)
end)

game.Players.PlayerRemoving:Connect(function(player) -- // Handle connections/delete frame on leaderboard
	if connections[player] then
		connections[player]:Disconnect()
	end
	if script.Parent:FindFirstChild(player.Name) then
		script.Parent:FindFirstChild(player.Name):Destroy()
	end
end)

toggleLeaderboard() -- // Open the leaderboard when the script runs

uis.InputBegan:Connect(function(input,gpe)
	if gpe then return end -- // Don't run when player is typing
	if input.KeyCode == KeyCode then
		toggleLeaderboard() -- // Toggle the leaderboard when a player presses tab
	end
end)

[yes35go]

1. As far as I can tell, your code looks free from leaks
2. Theoretically, you could use bool and X or Y so open and 0 or -1, but this may make your code harder to read, so you could make a position variable like this
   This would only help readability

open = not open
local xpos
if open then
  xpos = 0
else
  xpos = -1
end
script.Parent:TweenPosition(
  UDim2.new(0.289, 0, xpos, 0),
  Enum.EasingDirection.Out,
  Enum.EasingStyle.Linear,
  .5,
  true
)

But to use both the ideas

open = not open
xpos = open and 0 or -1
script.Parent:TweenPosition(
  UDim2.new(0.289, 0, xpos, 0),
  Enum.EasingDirection.Out,
  Enum.EasingStyle.Linear,
  .5,
  true
)

Its possible to just put that BOOL and X or Y statement right in the function call, but considering this is Code Review, I will say, in my opinion, that would make the code harder to read

3. One potential design improvement could be to have one server script check for any changes in leaderstats, then send a remote event to all the clients, rather than every player having every each signal connected to every other player. I’m not sure how much this matters in reality but it would decrease the amount of connections from ~n^2 (its actually n*(n-1) I believe) to 2n where n is the number of players on (so for 10 players, either 90 or 20 connections)
   This might help for code readability too, as you wouldn’t have to do connections within connections

Personally, I think code is easier to read if all code outside of functions is put at the end of the script, as I can don’t miss small pieces of code throughout function definitions

[Katrist]

You shouldn’t use :FindFirstChild again when you can just save the first check as a variable. Another thing you should do is set the connection index to nil, so the connections table doesn’t get overflowed.
You should also just save the players service as a variable.

Optimized code:

-- NotBroham

--//Services
local Players = game:GetService("Players")
local UserInputService = game:GetService("UserInputService")

-- // Variables \\ --
local template = script:WaitForChild('TEMPLATE') -- // The frame's template

--//Controls
local thumbnailurl = "https://www.roblox.com/bust-thumbnail/image?userId=%s&width=420&height=420&format=png" -- // Replace %s with a userid to get their avatar thumbnail
local open = false
local KeyCode = Enum.KeyCode.Tab

--//Tables
local connections = {} -- // Table so we can disconnect connections when we're done with them

-- // Functions/Loops \\ -- 
local function newPlayer(player) -- // Add a new player to the leaderboard/handle connections
	local leaderstats = player:WaitForChild('leaderstats')
	local Kills = leaderstats:WaitForChild("Kills")
	
	local cl = template:Clone()
	cl.Name = player.Name
	cl.TextLabel.Text = Kills.Value
	cl.Parent = script.Parent
	
	local id = Players:GetUserIdFromNameAsync(player.Name)
	cl.ImageLabel.Image = string.format(thumbnailurl, id)

	connections[player] = Kills.Changed:Connect(function(new)
		cl.LayoutOrder = -new -- // Order the leaderboard from most to least kills
		cl.TextLabel.Text = new
	end)
end

local function toggleLeaderboard() -- // Open/Close the leaderboard
	open = not open
	
	if open then -- // Wondering if I could do this without having to use a variable
		script.Parent:TweenPosition(UDim2.fromScale(0.289, 0), Enum.EasingDirection.Out, Enum.EasingStyle.Linear, 0.5, true)
	else
		script.Parent:TweenPosition(UDim2.fromScale(0.289, -1), Enum.EasingDirection.Out, Enum.EasingStyle.Linear, 0.5, true)
	end
end

for i, player in next, Players:GetPlayers() do -- // Loop for the players who joined before us/ourself
	newPlayer(player)
end

Players.PlayerAdded:Connect(function(player) -- // I could do game.Players.PlayerAdded:Connect(newPlayer) but I think this looks better
	newPlayer(player)
end)

Players.PlayerRemoving:Connect(function(player) -- // Handle connections/delete frame on leaderboard
	local playerConnection = connections[player]
	
	if playerConnection then
		playerConnection:Disconnect()
		
		connections[player] = nil
	end
	
	local uiFound = script.Parent:FindFirstChild(player.Name)
	
	if uiFound then
		uiFound:Destroy()
	end
end)

toggleLeaderboard() -- // Open the leaderboard when the script runs

UserInputService.InputBegan:Connect(function(input, gameProcessedEvent)
	if gameProcessedEvent then
		return
	end
	
	if input.KeyCode == KeyCode then
		toggleLeaderboard() -- // Toggle the leaderboard when a player presses tab
	end
end)