How can I improve my code?

Help and Feedback Code Review
#1 
game.ReplicatedStorage.VIPEvent.OnServerEvent:Connect(function(player, stats, player2, info, ...)
	local serverOwner = game.PrivateServerOwnerId
	local serverId = game.PrivateServerId
    local args = {...};
	if stats == "Team" then
		if player.UserId == serverOwner or args[1] == "Gamepass" and not args[1] == "TheAdministrator" then
		print("[AREA-11 VIP PANEL]:"..player.Name.." changed "..player2.." team to "..info.."!")
		local plr= game.Players:FindFirstChild(player2)
		plr.Team = game.Teams:FindFirstChild(info)
		wait(2)
		local plrmodel = game.Workspace:FindFirstChild(player2)
			plrmodel.Head:Destroy() -- Resets them
		elseif args[1] == "TheAdministrator" and not player.UserId == serverOwner and not args[1] == "Gamepass" or not player.UserId == serverOwner and args[1] == "TheAdministrator" then
			if args[1] == "TheAdministrator" then
				if player.Team == game.Teams["The Administrator"] then
					if game.MarketplaceService:UserOwnsGamePassAsync(player, 12345) then
						--repeat same teaming functions
					end
				else
					game.ReplicatedStorage.Animation:FireClient(player, player.Name)
				end
			else
				game.ReplicatedStorage.Animation:FireClient(player, player.Name)
			end
			else
		game.ReplicatedStorage.Animation:FireClient(player, player.Name)
		end

Note. game.ReplicatedStorage.Animation:FireClient() is clearly just my anti exploit so ignore

3 Likes
#2

You can drop FindFirstChild(x) and instead use [x] in every case where you don’t explicitly check if the return was nil.

You should also cache your services in a local variable and fetch them via GetService as it’s common practice.

The formatting could also use some improvements, since a couple statements are indented too little or too much.

2 Likes
#3

I got confused and was looking on how to improve it, I have a general question, How is it better to use :GetService() then just plain old game.____?

1 Like
#4

Yes; it’s generally advised you localize services you’ll use a lot somewhere once, and that you fetch them with GetService. It’s more consistent, and some services too have different names than their ClassName so GetService is encouraged for them.

#5

Thanks, I will be doing that now!

I also revmaped my code

game.ReplicatedStorage.VIPEvent.OnServerEvent:Connect(function(player, ...)
local serverOwner = game.PrivateServerOwnerId
local serverId = game.PrivateServerId
local market = game:GetService("MarketplaceService")
local args = {...};
--[[
	ARGUMENTS BOOK:
0 - Player
1 - Stats
2 - Target	
3 - Team
4 -	Bypass
5 - ...
]]
	if args[1] == "Team" then
		if args[4] == "VIP" then
			if player.UserId == serverOwner then
			local target = game.Players:FindFirstChild(args[2])
			local team = game.Teams:FindFirstChild(args[3])
			if target == nil then
				game.ReplicatedStorage.CoreGui:FireClient(player, "Action Failed", "Targeted player can't be found, incorrect username or they disconnected. Make sure your typing the full username")
			    end
			if team == nil then
				game.ReplicatedStorage.CoreGui:FireClient(player, "Action Failed", "Targeted team can't be found, incorrect team name. Make sure your typing the full name of the team")
				end
				if not target == nil and not team == nil then
					print("[AREA-11 VIP PANEL]:"..player.Name.." changed "..target.Name.." team to "..team.Name.."!")
					target.Team = team
					wait(0.5)
					target.Character.Head:Destroy()
				end	
			else
				game.ReplicatedStorage.Animation:FireClient(player, player.Name)
			end
		elseif args[4] == "TheAdministrator" then
			if  market:UserOwnsGamePassAsync(player, 1222) then
			local target = game.Players:FindFirstChild(args[2])
			local team = game.Teams:FindFirstChild(args[3])
			if target == nil then
				game.ReplicatedStorage.CoreGui:FireClient(player, "Action Failed", "Targeted player can't be found, incorrect username or they disconnected. Make sure your typing the full username")
			    end
			if team == nil then
				game.ReplicatedStorage.CoreGui:FireClient(player, "Action Failed", "Targeted team can't be found, incorrect team name. Make sure your typing the full name of the team")
				end
				if not target == nil and not team == nil then
					print("[AREA-11 VIP PANEL]:"..player.Name.." changed "..target.Name.." team to "..team.Name.."!")
					target.Team = team
					wait(0.5)
					target.Character.Head:Destroy()
				end	
			else
				game.ReplicatedStorage.Animation:FireClient(player, player.Name)
			end
		end
	end
end)
#6

This is straight up bad advice. You should never use instance[x] to index child instance. Remember that [x] will select the member of an instance before selecting a child. If [x] were used in OP’s code, it would break if the players “Name”, “FindFirstChild”, or the number of other players named after instance members were playing the game.

Even so, it can’t be guaranteed that a child exists when you expect it to. Again, OP’s example could fail because the character may have fallen out of the world, which removes their model. Or player2 may have left the server while the event was replicating, so their character no longer exists.

Even so, the class of an instance can have members added to it in the future. The name of that child that you’re indexing now could become the name of a member in the future.

The right way to do it is to either use methods like FindFirstChild that allow you check if a child exists before using it, or just have references to the objects you’ll be using in the first place. If you need to use Player objects, get them from Players.PlayerAdded, which is guaranteed to pass a Player object. If you need characters, use Player.Character, which is guaranteed to be either the player’s character model, or nil if it doesn’t exist. Instance.new("TextButton") is guaranteed to continue being that GUI button you created from scratch; keep a reference to it if you’re going to use it. Prefer the options where an object is guaranteed to be what you expect it to be.

9 Likes
FindFirstChild and WaitForChild: Addressing common bad practices and habits of each
#7

I recently while adding all the other a if args[1] == “” then, I noticed the same thing your talking about and I fixed that issue ending the script giving player notification hey, this player disconnected or incorrect username spelled.