How do I make this function possibly cleaner and more efficient?

I’m making an interface animation where the frames become visible. This function is imperative because there are a lot of instances with different settings for transparency. This function does not have any problems, but I’m always looking forward for better methods to do a similar one.

I cache every transparency setting of UI instances into a table to use afterwards.

local function TweenCanvas(UI)
	local Cache = { }
	for _, UI in ipairs(UI:GetDescendants()) do
		if UI:IsA('Frame') and UI.BackgroundTransparency ~= 1 then
			Cache[UI] = UI.BackgroundTransparency
			UI.BackgroundTransparency = 1
			TweenService:Create(UI, Info, {BackgroundTransparency = Cache[UI]}):Play()
		elseif UI:IsA('TextLabel') or UI:IsA('TextButton') then
			Cache[UI] = UI.TextTransparency
			UI.TextTransparency = 1
			TweenService:Create(UI, Info, {TextTransparency = Cache[UI]}):Play()
		elseif UI:IsA('ImageLabel') or UI:IsA('ViewportFrame') then
			Cache[UI] = { UI.BackgroundTransparency, UI.ImageTransparency }
			UI.BackgroundTransparency, UI.ImageTransparency = 1, 1
			TweenService:Create(UI, Info, {BackgroundTransparency = Cache[UI][1], ImageTransparency = Cache[UI][2]}):Play()
		elseif UI:IsA('UIStroke') then
			Cache[UI] = UI.Transparency
			UI.Transparency = 1
			TweenService:Create(UI, Info, {Transparency = Cache[UI]}):Play()

	Cache = nil

You could create a table mapping all the relevant ui types to their respective transparency properties and then just create the tweens with data from that table. Also, what is the point of the cache? if your not referencing it from outside of the loop. Also, viewport frames don’t have an image transparency property I think it should be image label.

Like this:

local TransparencyTweenData = {
		UITypes = {"ImageButton", "ImageLabel"},
		Props = {"BackgroundTransparency", "ImageTransparency"}
	--Add the other UI types with their respective properties

local function TweenCanvas(UI)
	for _, Descendant in ipairs(UI:GetDescendants()) do
		for _, TweenData in ipairs(TransparencyTweenData) do
			for _, UIType in ipairs(TweenData.UITypes) do
				if (Descendant:IsA(UIType)) then
					local PropsCache = {}
					for _, Prop in ipairs(TweenData.Props) do
						PropsCache[Prop] = Descendant[Prop]
						Descendant[Prop] = 1

I wouldn’t necessarily call this more efficient (with the exception of the fact that the cache dictionary has no need to be outside of the loop) and by no means is there a right or wrong way to do this, but if I were to program this same thing I’d do it something like this:

local mapTypeToProperty = {
	['Frame'] = {'Background'};
	['TextLabel'] = {'Text'};
	['TextButton'] = {'Text'};
	['ImageLabel'] = {'Background', 'Image'};
	['ViewportFrame'] = {'Background', 'Image'};
	['UIStroke'] = {''};

local function tweenCanvas(ui)
	for _,obj in pairs(ui:GetDescendants()) do
		local properties = mapTypeToProperty[obj.ClassName];
		if (properties) then
			local propInfo = {};
			for _,property in pairs(properties) do
				local propName = property .. 'Transparency';
				propInfo.propName = obj[propName];
				obj[propName] = 1;
			tweenService:Create(obj, info, propInfo):Play();

I haven’t tested this but you get the idea - a dictionary to map each object type to its associated transparency properties and then go through these and play a tween.

I hope this was helpful :slight_smile:

EDIT: If the caching of properties is absolutely required for later use (I’m unsure as I don’t know about the rest of your code), you can just store the oldVal property inside a cache outside of the loop, but from the code provided I wasn’t sure whether or not it was required :+1:

EDIT 2: Updated code to make sure only a single tween is created for each object, rather than multiple for each different property (as @RemoDunks rightly pointed out).

1 Like

Your solutions similar to mine (in the spirit of mapping properties to class types); however, I believe OP’s current solution is actually slightly more efficient than yours as the way you’ve set up the data array you have to have a 3rd loop within your function (thus increasing time complexity of the algorithm to a cubic). I do agree that its much more visually pleasing and has improved readability though judging from the title I believe that OP is looking for efficiency (though I may be wrong).

Good contribution though - the concept you had with the data array was the right sort of idea but I’m unsure if the code provided answers OP’s question :smile:

1 Like

That kind of efficiency for a system like this negligible and would definitely be considered premature optimization.

1 Like

I know the difference is minor but the title stated by OP refers to efficiency, that’s all :+1:

1 Like

Also, I wouldn’t use your solution because you are creating unnecessary additional tweens.

1 Like

Efficiency can also be measured in different ways and the op also asked to make the code cleaner.

Thank you for pointing this out - I have fixed that now :+1:

As I said in my initial reply, I agree that your code is much more readable and nicer to look at. I’m just unsure if OP is willing to sacrifice one for the other, especially when both can be attained simultaneously :slightly_smiling_face:

As I also said the op isn’t really sacrificing any performance.

I mentioned earlier that I agree the difference is negligible and yes, though I agree it’s over the top considering it’s such a small task, it’s what OP requested in the title :smile:

I think you’re overall misunderstanding how computers work.

  1. Complexity is an asymptotic measure, it is meaningful only when the size of the parameters becomes significant enough compared to the computing power of a device. Additionally, a loop of fixed length is O(1), not O(n) like you’re saying.
  2. The code using a mapping table is also in constant time, since table property access is constant.
  3. Code is compiled, the compiler will do plenty of tweaks to your code to make it more efficient before it’s actually turned into machine code. Abstract your thoughts from what the code looks like under your eyes, and privilege readability over so-called optimisations.

Also just saying, @mayaAboveAll’s code is definitely cleaner.

  1. I may have been misusing big O (I’m unsure if I was) but the only reason why I expressed it in terms of n is because OP may be creating UI components dynamically and because I’m unsure on the number of contents that’d be within @RemoDunks’ TransparencyTweenData array and their associated contents too. I’m not sure if that allows me to express it in terms of n, as the array given would be of fixed length but the others may change change depending on the type and and object insertion. :ohmy: I appreciate that it’s also fairly insignificant for such a small task that involves little complexity :+1:

  2. Thank you for informing me of this, never realised the compiler actually tweaks code as was never taught that, was just told that it reads through and translates it into machine code :slight_smile: Always looking to learn something new!

1 Like

What is the basis for your claim that his code is cleaner? Some of the main flaws I see with their code:

  1. How they chose to reference the properties by appending “transparency” to the property has poor readability and maintainability, since Roblox may add another transparency property that does not follow the same format.

  2. For instances with the same properties, you would need to have unnecessary duplicate entries, like image buttons and image labels.

  3. Directly referencing the classname is generally regarded as poor practice since you lose the ability to check for abstract parent classes.

I kind of agree with all that you said. I claimed this version is better essentially because it’s using a map whereas, based on their example, it doesn’t seem like they’d have plenty of duplicates, making it more readable, but otherwise they’d choose the other version. Regarding the use of ClassNames, they could use IsA instead.