How to improve code?

[LoordySr (LoordySir)]

Dear scripters. I rarely see someone that want to improve a code. Anyways, let’s go straight to the point.

How can I improve this code in any way ?

local viewportFrame = playerGui.DeepScreen.Shop.ScrollingFrame.AccessoriesPage.ViewportFrame


local function createViewport(accessory)
	local viewportAccessory = accessory:Clone()
	local accessoryViewport = viewportFrame:Clone()
	local worldModel = accessoryViewport.WorldModel

	local VPFcam = Instance.new("Camera")
	VPFcam.Parent = accessoryViewport
	VPFcam.CFrame = CFrame.new(0, 0, 0)
	accessoryViewport.CurrentCamera = VPFcam

	viewportAccessory.Parent = accessoryViewport:WaitForChild("WorldModel")
	viewportAccessory.Handle.CFrame = CFrame.new(Vector3.new(0, 0, -2.3), Vector3.new(0, 0, 0))
	accessoryViewport.Name = viewportAccessory.Name
	
	accessoryViewport.MouseMoved:Connect(function()
		viewportAccessory.Handle.CFrame *= CFrame.fromEulerAnglesXYZ(0, 0.1, 0)
	end)
	
	accessoryViewport.InputBegan:Connect(function(input) --devforum how to improve code..
		if input.UserInputType == Enum.UserInputType.MouseButton1 then
			SoundService.UISFX.Equip2:Play()
			
			for _, charAccessory: Accessory in pairs(character:GetChildren()) do
				if charAccessory:IsA("Accessory") then
					if charAccessory.Name == accessory.Name or charAccessory.AccessoryType.Name == accessory.AccessoryType.Name then
						charAccessory.Parent = ReplicatedStorage.Accessories
						accessory.Parent = character
						
						accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(charAccessory.Name).BackgroundColor3 = Color3.new(1, 1, 1)
						
						accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(charAccessory.Name).UIStroke.Color = Color3.new(0, 0, 0)
						
					else
						accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
						accessory.Parent = character
						
					end
				elseif not character:FindFirstChildOfClass("Accessory") then
					accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
					accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
					accessory.Parent = character
				end
			end
			
		end
	end)

	return accessoryViewport
end

local function setUpViewports()
	local accessoriesFolder = ReplicatedStorage:WaitForChild("Accessories")

	for _, accessory in ipairs(accessoriesFolder:GetChildren()) do
		createViewport(accessory).Parent = viewportFrame.Parent
	end
end

The only improvment I actually want to learn is how to make this code better (shorter and nicer)

				if charAccessory:IsA("Accessory") then
					if charAccessory.Name == accessory.Name or charAccessory.AccessoryType.Name == accessory.AccessoryType.Name then
						charAccessory.Parent = ReplicatedStorage.Accessories
						accessory.Parent = character
						
						accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(charAccessory.Name).BackgroundColor3 = Color3.new(1, 1, 1)
						
						accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(charAccessory.Name).UIStroke.Color = Color3.new(0, 0, 0)
						
					else
						accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
						accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
						accessory.Parent = character
						
					end
				elseif not character:FindFirstChildOfClass("Accessory") then
					accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
					accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
					accessory.Parent = character
				end

Because it feels like

part.Transparency = 0.1
task.wait(0.1)
part.Transparency = 0.2
task.wait(0.1)
part.Transparency = 0.3
task.wait(0.1)
part.Transparency = 0.4
task.wait(0.1)
part.Transparency = 0.5
etc..

Thanks for your time. Goodluck flexing with your experience.

[thebigkannye]

Its hard to improve a script thats so confusingly written, when we as third parties don’t know what’s important or not.

However you do have three places where you set this nearly the exact same way

					accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
					accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)
					accessory.Parent = character

Just put that code below outside of the if statements.

[wowland856 (wowland)]

Something immediately apparent is you are setting the parent of instances before setting properties:

local VPFcam = Instance.new("Camera")
VPFcam.Parent = accessoryViewport
VPFcam.CFrame = CFrame.new(0, 0, 0)

This is bad practice, you should always set properties before parenting an instance. This post into more detail and explains it better than I could as to why we do this:

PSA: Don't use Instance.new() with parent argument

I’ve discovered a pretty bad performance issue in one of top games that has to do with Instance.new and wanted to write about this, since this is not obvious unless you know the system inside out. Tthere are several ways to create a ROBLOX object in Lua: local obj = Instance.new(‘type’); fill obj fields local obj = Instance.new(‘type’, parent); fill obj fields local obj = util.Create(‘type’, { field1 = value1, … }) If you care at all about performance, please only use the first option - I wi…

[ZerroxShiot (Zerrox)]

Something like this I guess?
I haven’t tested the code though.

for _, charAccessory in pairs(character:GetChildren()) do
    local sameName = charAccessory.Name == accessory.Name
    local sameType = charAccessory.AccessoryType.Name == accessory.AccessoryType.Name
    local isAccessory = charAccessory:IsA("Accessory")

    if isAccessory and (sameName or sameType) then
        charAccessory.Parent = ReplicatedStorage.Accessories
        accessoryViewport.Parent:FindFirstChild(charAccessory.Name).BackgroundColor3 = Color3.new(1, 1, 1)
        accessoryViewport.Parent:FindFirstChild(charAccessory.Name).UIStroke.Color = Color3.new(0, 0, 0)
    end

    if isAccessory or not character:FindFirstChildOfClass("Accessory") then
        accessoryViewport.Parent:FindFirstChild(accessory.Name).BackgroundColor3 = Color3.new(0.266667, 1, 0)
        accessoryViewport.Parent:FindFirstChild(accessory.Name).UIStroke.Color = Color3.new(0.266667, 1, 0)

        accessory.Parent = character
    end
end