How can I make this code cleaner?

Help and Feedback Code Review
1

Hello everyone, how can I make the following code cleaner?

local Cross = {}

Cross.Admins = {game.CreatorId} -- Who is admined? (IDs only, the owner is already set.)
Cross.bannedUsers = {
	"ROBLOX" -- View the DevForum guide for more informations about this.
}
Cross.debugingPerks = false -- Will achdef have admin for debbuging?

game.Players.PlayerAdded:Connect(function(player)
	if table.find(Cross.Admins, player.UserId) then
		local templateButton = game:GetService("ReplicatedStorage").Cross.UIs.Cross.OpenButton
		local button = templateButton:Clone()
		button.Text = "Open Cross"
		button.Color3 = Color3.new(0.333333, 1, 0)
	elseif table.find(Cross.Admins, player.UserId) == nil then
		local templateButton = game:GetService("ReplicatedStorage").Cross.UIs.Cross.OpenButton
		local button = templateButton:Clone()
	end
end)

return Cross

I know it’s really messy, that why I posted this. Help is appreciated!

1 Like
2

You don’t need to use table.find you can just do

table.admins[player.UserId]
3

A few things

  • You’re repeating the templateButton variable twice whe nyou could just put it outside of the event
  • Why are you using elseif in this case? There can only be one other thing besides if they were found, they weren’t found, so else is better. But in this case it’s unneeded since you can put the button clone part as the first thing when the player joins considering it’s in both statements anyways
  • You aren’t parenting the button to anywhere so the cloning is kinda useless unless you’re planning to do that later

This would be cleaner

local Cross = {}

Cross.Admins = {game.CreatorId} -- Who is admined? (IDs only, the owner is already set.)
Cross.bannedUsers = {
	"ROBLOX" -- View the DevForum guide for more informations about this.
}
Cross.debugingPerks = false -- Will achdef have admin for debbuging?

local templateButton = game:GetService("ReplicatedStorage").Cross.UIs.Cross.OpenButton

game.Players.PlayerAdded:Connect(function(player)
	local button = templateButton:Clone()
	if table.find(Cross.Admins, player.UserId) then
		button.Text = "Open Cross"
		button.Color3 = Color3.new(0.333333, 1, 0)
	end
end)

return Cross

@elonrocket The Cross.Admins thing is a table, not a dictionary, so table.find is needed

4

Do you just mean?

admins[player.UserId] = true
5

Yes you can do

if admins[player.UserId] = true then 
end

also what are the gui’s all about? this is a ServerScript so the the gui’s are pointless.

6

Oh, right. The button is in StarterGui, I removed the clone right now.

7

They are not when making our own panel. Also, my table is not a dictionnary.

8

Oh I didn’t notice that, in that case I recommend saving it as a dictionary with player.UserId as the key.

9

It still would be slightly unneeded since the performance increase of using dictionary lookup vs table.find is marginal unless someone decides to have 1000 admins. it’s better if it’s left as a table for simplicity, unless that slight but unnoticeable in practice speed increase is wanted by @OP

11

I’m talking about simplicity, imo it’s much more complicated to run through functions to find things in an array than to simply lookup a key using table[key]

12

Here is my implementation

#1 (What I recommend)

local Players = game:GetService('Players')
local ReplicatedStorage = game:GetService('ReplicatedStorage')

local Template = ReplicatedStorage.Cross.UIs.Cross.OpenButton
local Cross = {}

Cross.Admins = {game.CreatorId} -- Who is admined? (IDs only, the owner is already set.)
Cross.BannedUsers = {
	"ROBLOX" -- View the DevForum guide for more informations about this.
}
Cross.DebugingPerks = false -- Will achdef have admin for debbuging?

local function OnPlayerAdded(Player) --> Void
	local IsAdmin = table.find(Cross.Admins, Player.UserId)
	local Clone = Template:Clone()
	
	if IsAdmin then
		Clone.Color3 = Color3.new(0.333333, 1, 0)
		Clone.Text = "Open Cross"
	end
	return nil
end

for _, Player in ipairs(Players:GetPlayers()) do
	OnPlayerAdded(Player)
end
Players.PlayerAdded:Connect(OnPlayerAdded)
return Cross

#2

local Players = game:GetService('Players')
local ReplicatedStorage = game:GetService('ReplicatedStorage')

local Template = ReplicatedStorage.Cross.UIs.Cross.OpenButton
local function SetAndBag(Array) --> Dictionary
	local Copied = {}
	for _, Value in ipairs(Array) do
		Copied[Value] = true
	end
	return Copied
end
local Cross = {}

Cross.Admins = SetAndBag({game.CreatorId}) -- Who is admined? (IDs only, the owner is already set.)
Cross.BannedUsers = SetAndBag({
	"ROBLOX" -- View the DevForum guide for more informations about this.
})
Cross.DebugingPerks = false -- Will achdef have admin for debbuging?

local function OnPlayerAdded(Player) --> Void
	local IsAdmin = Cross.Admins[Player.UserId]
	local Clone = Template:Clone()
	
	if IsAdmin then
		Clone.Color3 = Color3.new(0.333333, 1, 0)
		Clone.Text = "Open Cross"
	end
	return nil
end

for _, Player in ipairs(Players:GetPlayers()) do
	OnPlayerAdded(Player)
end
Players.PlayerAdded:Connect(OnPlayerAdded)
return Cross

Just like @EmbatTheHybrid mentioned, a dictionary lookup vs table.find is in most scenarios, a premature optimization (especially in game development) but I just included a dictionary version just cuz 0_0. (Manually returning nil is completely optional. I do it for improved readability)

Useful Sources:
Sets and Bags :smiling_imp:
Roblox: Doing PlayerAdded Correctly :cold_face:

2 Likes