Guard clauses are causing the first part of my setting code to be unreadable

[7z99 (cody)]

So my code is currently pretty unreadable to me. Currently what it does is simply set, read from, and initialize settings. What I currently dislike about my code is that it is basically unreadable, for me anyway.

For context, our options panel is laid out as follows:

> Column
    > Setting Category Panels
        > Container (holds list of items)
            > List of settings & controllers

Because of these deep nests of options and the need to iterate through each and every one sort of recursively, it causes the need for deep nests of loops.

To reduce the number of indents, I’ve resorted to using guard clauses to skip to next iterations because I also need to check the class of each variable at each level of indent.

So because of this I run into 4 levels of

for i, name in ipairs(whatever:GetChildren()) do
    if not name:IsA(class) then
        continue
    end
    ...

And because they all have similar names, it gets confusing, very quickly. It’s functional, and I’m satisfied with performance, but I really don’t know how I’d improve the readability of it.

Anyway, prepare your eyes:

	for i, column in ipairs(container:GetChildren()) do
		if not column:IsA('Frame') then
			continue
		end
		for i2, settingCategory in ipairs(column:GetChildren()) do
			if not settingCategory:IsA('Frame') then
				continue
			end
			local foundSettingCategory = module._settingsInfo[settingCategory.Name]
			if not foundSettingCategory then
				return warn(`No setting category entry for {settingCategory}!`)
			end
			for i3, settingControlContainer in ipairs(settingCategory:WaitForChild('Container'):GetChildren()) do
				if not settingControlContainer:IsA('Frame') then
					continue
				end
				local thisInformation = foundSettingCategory[settingControlContainer.Name]
				if not thisInformation then
					warn(`No setting entry found for {settingControlContainer:GetFullName()}!`)
					continue
				end

Thanks in advance for any guidance. Let me know if you have any questions.

[Stratiz]

You’re right; this is pretty rough in terms of readability.

So right off the bat, I’m noticing three things:

1. You’re repeating the same conditional logic of if instance is frame then in 3 places. When we have repeat code, converting It into a function is generally good practice.
2. Your code is super scrunched up. Scrunched code is always harder to read compared to smaller chunks.
3. You have unused variables.

I’d address the first concern in two ways to make this more readable.

1. Turn the guard clause into a function:

local function getFrames(frameContainer : Instance)
    local frames = {}
    for _, frame in pairs(frameContainer:GetChildren()) do
        if frame:IsA('Frame') then
            table.insert(frames, frame)
        end
    end
    return frames
end

--[[ You could drop the "ipairs" here too, Luau will automatically run pairs for you:
   Unless you need ipairs specifically, the following is valid in Luau.

   local t = {1, 2, 3}
   for i, v in t do
      --stuff
   end
--]]
for i, column in ipairs(getFrames(container)) do
	for i2, settingCategory in ipairs(getFrames(column)) do
		local foundSettingCategory = module._settingsInfo[settingCategory.Name]
		if not foundSettingCategory then
			return warn(`No setting category entry for {settingCategory}!`)
		end
		for i3, settingControlContainer in ipairs(getFrames(settingCategory:WaitForChild('Container'))) do
			local thisInformation = foundSettingCategory[settingControlContainer.Name]
			if not thisInformation then
				warn(`No setting entry found for {settingControlContainer:GetFullName()}!`)
				continue
			end

If you want to get fancy and optimize:

2. Make your own iterator function like so:

function childFrames(frameParent : Instance)
    assert(typeof(frameParent) == "Instance", "frameParent must be of type instance")
    local index = 0
    local children = frameParent:GetChildren()
    local childCount = #children
    return function()
        while index < childCount do
            index += 1
            if children[index]:IsA("Frame") then
                return children[index]
            end
        end
        return
    end
end

for i, column in childFrames(container) do
	for i2, settingCategory in childFrames(column) do
		local foundSettingCategory = module._settingsInfo[settingCategory.Name]
		if not foundSettingCategory then
			return warn(`No setting category entry for {settingCategory}!`)
		end
		for i3, settingControlContainer in childFrames(settingCategory:WaitForChild('Container')) do
			local thisInformation = foundSettingCategory[settingControlContainer.Name]
			if not thisInformation then
				warn(`No setting entry found for {settingControlContainer:GetFullName()}!`)
				continue
			end

To address the second point, un-scrunching your code will make it much more readable. There’s no definitive way to do this; use your discretion.

Here’s an example of scrunched vs. un-scrunched code:

Scrunched

local t = {1}
table.insert(t, 2)
table.insert(t, 3)
table.insert(t, 4)
local newTable = {}
for i,v in t do
    if i % 2 == 0 then
         table.insert(newTable, v)
    end
end
print("Result", newTable)

Un-scrunched

local t = {1}

table.insert(t, 2)
table.insert(t, 3)
table.insert(t, 4)

local newTable = {}

for i,v in t do
    if i % 2 == 0 then
         table.insert(newTable, v)
    end
end

print("Result", newTable)

See how much easier it is to read the unscrunched code?

For the 3rd and final point, unused variables are distracting. In Lua, if you’re not using a variable that’s returned as part of a tuple (such as i & v from pairs), its good practice to substitute them with an underscore like so:

local t = {1, 2, 3}

for _, value in ipairs(t) do --i or index is unused so we replaced it with _
   print("Value is", value)
end

Roblox’s intellesense will also recognize _ as a placeholder variable:

[7z99 (cody)]

That’s perfect, thank you! I’ll readdress those points a bit later, that looks like it will help a lot.

I had a question for the scrunched thing though. Based on what you’ve provided, it looks like you separate variable declarations, function calls, and keep short loops/if statements as a single compact block. The only thing I was wondering was what you’d do in the case where you’re switching between each of those 3 “categories” so your code would be pretty spaced out? For example here in this module, using that convention would this be too spaced out or would you eliminate some of the spaces, or would you completely restructure it?

local function setTextAndColour(oldValue, value)
	local value = if value ~= nil then value else optionsHandler:GetOptionValue(settingControlContainer.Name).Value

	if value == nil then
		warn('No value', settingControlContainer)
		return
	end

	if thisInformation.InverseBoolean then
		value = not value
	end

	local textOverrideType = module.TextTypeOverrides.boolean[thisInformation.TextOverrideType or 'Default'][value]

	if not textOverrideType then
		warn(`TextTypeOverrides.{thisInformation.TextOverrideType or 'Default'}.{value} does not exist! (Setting name {settingControlContainer})`)
	end

	local colourOverride = if thisInformation.ColourOverrides then thisInformation.ColourOverrides[value] elseif value then GREEN else RED

	buttonObject:SetText(textOverrideType)
	buttonObject:SetColour(colourOverride)
end

@Stratiz

--[[ You could drop the "ipairs" here too, Luau will automatically run pairs for you:
   Unless you need ipairs specifically, the following is valid in Luau.

   local t = {1, 2, 3}
   for i, v in t do
      --stuff
   end
--]]

Just wanted to respond to this, I really only used generalized iteration in place of pairs instead of ipairs as I’ve heard due to Luau optimization ipairs is actually faster than pairs (vs in normal Lua, ipairs is slower than pairs). I never really benchmarked it until now but I just did that and turns out the difference is very negligible, in some cases GI is better than stock iteration functions and in some cases stock is better, so I think I’ll switch over to GI in place of ipairs. Thanks for that!

[Stratiz]

@7z99

using that convention would this be too spaced out or would you eliminate some of the spaces, or would you completely restructure it?

Where I work, this is generally the ideal structure. There’s been much debate about how you should un-scrunch, but the way you have it is pretty much what the team has landed on (+ comments and documentation)

It feels unusual initially, but it’s easier to read and makes onboarding developers much quicker. I recommend doing whatever you and your team feels is most readable.

Very insightful benchmark you’ve done on GI, pairs & ipairs. I always assumed GI was essentially using pairs without the name call. I’d be interested in finding out where the optimization is.

[7z99 (cody)]

@Stratiz

I’d be interested in finding out where the optimization is.

The RFC talks about it here, particularly the end of the default table iteration section:
GitHub

Particularly:

To ensure that this traversal is performant, the actual implementation of the traversal involves going over the array part (in index order) and then over the hash part (in hash order). For that implementation to satisfy the criteria above, we need to make two additional changes to table insertion/rehash:

• When inserting key k in the table when k == t->sizearray + 1 , we force the table to rehash (resize its array portion). Today this is only performed if the hash portion is full, as such sometimes numeric keys can end up in the hash part.
• When rehashing the table, we ensure that the hash part doesn’t contain the key newsizearray + 1 . This requires checking if the table has this key, which may require an additional hash lookup but we only need to do this in rare cases based on the analysis of power-of-two key buckets that we already collect during rehash.

These changes guarantee that the order observed via standard traversal with next / pairs matches the guarantee above, which is nice because it means we can minimize the complexity cost of this change by reusing the traversal code, including VM optimizations. They also mean that the array boundary (aka #t ) can always be computed from just the array portion, which simplifies the table length computation and may slightly speed it up.