Help Optimizing my code and making it more Efficient

Help and Feedback Code Review
#1

So I decided to make this Trail Script for VIP users in a game I’ve been working on for a long time now. I was wondering if I could make this code more efficient so it’s not such a big long script. I know this prob doesn’t look the best but It works for me. Explaining what it does, so if I was to do the command “!trail white” in chat it would give me a trail and if I did “!untrail” it would remove the trail. if there is nothing else that can be done its fine. I just think it was all kind of a mess lol

local Command1 = "!trail white"
local Command2 = "!trail red"
local Command3 = "!trail blue"
local Command4 = "!trail purple"
local Command5 = "!trail pink"
local Command6 = "!trail black"
local Command7 = "!trail green"
local Command8 = "!trail yellow"

local UnCommand = "!untrail"
local MarketPlaceService = game:GetService("MarketplaceService")
local gamepass = 1 --[[Just had to remove it]]--


game.Players.PlayerAdded:Connect(function(player)
	player.CharacterAdded:Connect(function(chr)
		player.Chatted:Connect(function(msg)

			local PlayerBoughtThisGamePass =  MarketPlaceService:UserOwnsGamePassAsync(player.UserId, gamepass)
			local trail = player.Character:FindFirstChild('Trail')
			local newTrail = Instance.new('Trail')
			if PlayerBoughtThisGamePass then


				if msg == Command1 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(255, 255, 255), Color3.fromRGB(255, 255, 255))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(255, 255, 255), Color3.fromRGB(255, 255, 255))

						end
					end)
				end

				if msg == Command2 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(255, 0, 0), Color3.fromRGB(255, 0, 0))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(255, 0, 0), Color3.fromRGB(255, 0, 0))
						end
					end)
				end

				if msg == Command3 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(0, 170, 255), Color3.fromRGB(0, 170, 255))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(0, 170, 255), Color3.fromRGB(0, 170, 255))
						end
					end)
				end

				if msg == Command4 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(0, 0, 127), Color3.fromRGB(0, 0, 127))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(0, 0, 127), Color3.fromRGB(0, 0, 127))
						end
					end)
				end

				if msg == Command5 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(170, 85, 127), Color3.fromRGB(170, 85, 127))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(170, 85, 127), Color3.fromRGB(170, 85, 127))
						end
					end)
				end

				if msg == Command6 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(0, 0, 0), Color3.fromRGB(0, 0, 0))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(0, 0, 0), Color3.fromRGB(0, 0, 0))
						end
					end)
				end

				if msg == Command7 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(0, 255, 0), Color3.fromRGB(0, 255, 0))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(0, 255, 0), Color3.fromRGB(0, 255, 0))
						end
					end)
				end

				if msg == Command8 then
					pcall(function()

						if player.Character:FindFirstChild('Trail') then

							trail.Color = ColorSequence.new(Color3.fromRGB(255, 255, 0), Color3.fromRGB(255, 255, 0))

						else

							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color3.fromRGB(255, 255, 0), Color3.fromRGB(255, 255, 0))
						end
					end)
				end


				if msg == UnCommand then
					pcall(function()

						player.Character:FindFirstChild('Trail'):Destroy()

					end)
				end
			end
		end)
	end)
end)
#2

You can use for commands (and tail color) module script and make this script more more smaller.

1 Like
#3

here is codes:

–Module Script

local module = {}

module["!trail white"] = Color3.fromRGB(255, 255, 255)
module["!trail red"] = Color3.fromRGB(255, 0, 0)
module["!trail blue"] = Color3.fromRGB(0, 170, 255)
module["!trail purple"] = Color3.fromRGB(0, 0, 127)
module["!trail pink"] = Color3.fromRGB(170, 85, 127)
module["!trail black"] = Color3.fromRGB(0, 0, 0)
module["!trail green"] = Color3.fromRGB(0, 255, 0)
module["!trail yellow"] = Color3.fromRGB(255, 255, 0)

return module

–Server Script

local MarketPlaceService = game:GetService("MarketplaceService")
local ModuleScript = require(script.Parent.ModuleScript)
local UnCommand = "!untrail"
local gamepass = 1 --[[Just had to remove it]]--


game.Players.PlayerAdded:Connect(function(player)
	player.CharacterAdded:Connect(function(chr)
		player.Chatted:Connect(function(msg)
			local PlayerBoughtThisGamePass =  MarketPlaceService:UserOwnsGamePassAsync(player.UserId, gamepass)
			local trail = player.Character:FindFirstChild('Trail')
			local newTrail = Instance.new('Trail')
			if PlayerBoughtThisGamePass then
				if ModuleScript[msg] then
					local Color = ModuleScript[msg] 
					pcall(function()
						if player.Character:FindFirstChild('Trail') then
							trail.Color = ColorSequence.new(Color, Color)
						else
							newTrail.Parent = player.Character
							newTrail.Attachment0 = player.Character.HumanoidRootPart.RootRigAttachment
							newTrail.Attachment1 = player.Character.Head.NeckRigAttachment
							newTrail.Color = ColorSequence.new(Color, Color)
						end
					end)
				end
				if msg == UnCommand then
					pcall(function()
						player.Character:FindFirstChild('Trail'):Destroy()
					end)
				end
			end
		end)
	end)
end)

image

2 Likes
#4

I would have approached it in a slightly different manner. The risk you have is that every time a player respawns this event is going to Connect again:

player.CharacterAdded:Connect(function(chr)

But do you ever Disconnect it on character death. If you don’t then there are potential memory leaks from unused/orphaned instances I believe (somebody feel free to contradict me).

I would approach in this rough manner:

-- FUNCTION
local function onPlayerChatted(player, message)
	local playerHasGamePass = player:GetAttribute(gamepass) -- No need to check UserOwnsGamePassAsync, we've already done that
	playerHasGamePass if not playerHasGamePass then return end -- If they dont own it, stop
	local chr = player.Character-- Now get teh Character
	local trail = chr:FindFirstChild('Trail') -- Notice we are re-using values we have already found
	if not trail then -- Check once if a trail exists, if not, create it
		trail = Instance.new('Trail')
	end
	if msg == Command1 then
		-- your stuff
	end
	if msg == Command2 then
		-- other stuff
	end
	-- etc

end

-- EVENTS
-- Add an Attribute to the Player to confirm they own the GP
game.Players.PlayerAdded:Connect(function(player)
	local PlayerBoughtThisGamePass =  MarketPlaceService:UserOwnsGamePassAsync(player.UserId, gamepass)
	if PlayerBoughtThisGamePass then
		player:SetAttribute(gamepass, true)
	end

-- Capture the chatter events and do trail stuff
	player.Chatted:Connect(function (...)
		onPlayerChatted(player, ...)
	end)
end)

It stops you repeatedly checking if the Player owns the pass. Do it once and save an Attribute to the player.