Optimising code to be less clustered

Help and Feedback Scripting Support
#1 
checkToys.OnClientEvent:Connect(function(...)
	local toysModule = require(replicatedStorage.Toys)
	
	local tuple = {...}
	inner:FindFirstChild('Teddy Bear').Quantity.Text = tuple[1]
	inner['Teddy Bear'].Price.Text = '$' .. toysModule.Selling['Teddy Bear']
	if tuple[1] > 0 then
		inner['Teddy Bear'].ImageColor3 = Color3.fromRGB(210, 50, 50)
	else
		inner['Teddy Bear'].ImageColor3 = Color3.fromRGB(150, 150, 150)
	end
	inner:FindFirstChild('Ball').Quantity.Text = tuple[2]
	inner['Ball'].Price.Text = '$' .. toysModule.Selling['Ball']
	if tuple[2] > 0 then
		inner['Ball'].ImageColor3 = Color3.fromRGB(210, 50, 50)
	else
		inner['Ball'].ImageColor3 = Color3.fromRGB(150, 150, 150)
	end
	inner:FindFirstChild('Yo-Yo').Quantity.Text = tuple[3]
	inner['Yo-Yo'].Price.Text = '$' .. toysModule.Selling['Yo-Yo']
	if tuple[3] > 0 then
		inner['Yo-Yo'].ImageColor3 = Color3.fromRGB(210, 50, 50)
	else
		inner['Yo-Yo'].ImageColor3 = Color3.fromRGB(150, 150, 150)
	end
	inner:FindFirstChild('Slinky').Quantity.Text = tuple[4]
	inner['Slinky'].Price.Text = '$' .. toysModule.Selling['Slinky']
	if tuple[4] > 0 then
		inner['Slinky'].ImageColor3 = Color3.fromRGB(210, 50, 50)
	else
		inner['Slinky'].ImageColor3 = Color3.fromRGB(150, 150, 150)
	end
end)

I currently have this script, which looks very ‘ugly’ and is hard to read at most times. Basically when an event fires it should show how much of each toy a player has (being the tuple) but the problem is I have to go through every single toy and set the corresponding tuple to it. Is there a way I can do this using tables or for loops somehow?

#3

Typically when you have a bunch of repeated code like this it is a good clue that you should be using a function. You are doing the same thing for each toy, so you can make a function that takes a toyName and count and then updates the Gui for that toy.

e.g


function updateToyCount(toyName, newCount)
	local toyGui = inner:FindFirstChild(toyName)
	if toyGui then
		toyGui.Quantity.Text = newCount
		toyGui.Price.Text = '$' .. toysModule.Selling[toyName]
		if newCount > 0 then
			toyGui.ImageColor3 = Color3.fromRGB(210, 50, 50)
		else
			toyGui.ImageColor3 = Color3.fromRGB(150, 150, 150)
		end
	end
end

checkToys.OnClientEvent:Connect(function(...)
	local toysModule = require(replicatedStorage.Toys)
	
	local tuple = {...}
	updateToyCount('Teddy Bear', tuple[1])
	updateToyCount('Ball', tuple[2])
	 --- ect.
end)

Another good practice is using local variables to decrease the amount of overly verbose statements. For example in the code above I use toyGui instead of inner[‘Toy name here’] again and again. This makes it easier to read and modify in the future.

6 Likes
#4

You can also turn:

if newCount > 0 then
    toyGui.ImageColor3 = Color3.fromRGB(210, 50, 50)
else
	toyGui.ImageColor3 = Color3.fromRGB(150, 150, 150)
end

to:

 toyGui.ImageColor3 = newCount > 0 and Color3.fromRGB(210, 50, 50) or Color3.fromRGB(150, 150, 150)
2 Likes
#5

Just saying - this is what #development-support:requests-for-code-feedback is for.