Rainbow Items Script From client

Help and Feedback Code Review
2

It’s hard to give feedback on such a small script, but here’s some tips.

  1. Directly appending a new key like RainbowWood[#RainbowWood + 1] runs faster than calling table.insert. Same concept applies for removing a table key. Instead of using table.remove, RainbowWood[key] = nil is more efficient.

  2. Instead of having to create a new thread with coroutines costing resources, you can just put that while loop at the end of the script, so it doesn’t yield the rest of the code that needs to run.

  3. ‘wait()’ is a big code smell. Instead of calling that to yield your code, it’s better to use ‘runservice.Heartbeat:Wait()’. If you want faster checks it’s better to use ‘Heartbeat:Wait()’ since it has been benchmarked to run around 2x faster than ‘wait()’. There are some other points listed in the post I linked about how wait can sometimes be delayed, etc…

Avoiding wait() and why

  1. Do stick with consistent casings when naming your variables whether its PascalCase or camelCase.
    Example: In your coroutine thread, you have a numeric loop in another numeric loop. For the outer - scoped one, you use ’ i ’ and in the loop inside that, you use ’ A '. This does not impact code performance at all, I’m just suggesting tips when it comes to readability.

  2. I don’t recommend using numerical loops over the generic loop (ipairs). Yes, numeric runs faster than ipairs but when it comes to retrieving the value it really isn’t. When you use a numeric loop you have to do ‘RainbowWood[A]’ to retrieve the value which is not instantaneous. However, the generic loop caches the value in a variable (for i, v in ipairs(array) do).

  3. I don’t understand the logic of this chunk of code

				if RainbowWood[A] then
					RainbowWood[A].Color = ComputedColor
				else
					table.remove(RainbowWood,A)
				end

If the key ‘A’ does not exist in your array, why would u remove it?

I’m not sure how collection service works, but here is how I would rewrite your code:

local CollectionService = game:GetService("CollectionService")
local RunServiceHeartbeat = game:GetService('RunService').Heartbeat

local RainbowWood = {}
local Speed = 3

for _, Object in pairs(CollectionService:GetTagged("RainbowWood")) do
	RainbowWood[#RainbowWood + 1] = Object
end

CollectionService:GetInstanceAddedSignal("RainbowWood"):Connect(function(Object)
      RainbowWood[#RainbowWood + 1] = Object
end)

CollectionService:GetInstanceRemovedSignal("RainbowWood"):Connect(function(Object)
      RainbowWood[Object] = nil
end)

CollectionService:GetInstanceAddedSignal("RainbowWood") 
CollectionService:GetInstanceRemovedSignal("RainbowWood")

while true do
	for i = 0, 1, 0.001 * Speed do
		local ComputedColor = Color3.fromHSV(i, 1, 1)
		for _, v in ipairs(RainbowWood) do
			v.Color = ( v and ComputedColor ) or v.Color
		end
	end
	RunServiceHeartbeat:Wait()
end

Due to the 6th reason I had, I decided not to include the part where you remove A from RainbowWood

2 Likes