Fixing and cleaning up large if/then walls

GibusWielder · November 11, 2022, 3:23pm

So recently, I came across a problem… I am making a combat system, and because, well, script kiddies, I need to check if anything is nil before proceeding. This is… kinda what that looked like:

if character then
    if character.HumanoidRootPart then
        if character.Humanoid then
            -- and so on...
        end
    end
end

That gets ugly fast. So I made a creative workaround, which is much more readable! Using a pcall and assert, we can check if a bunch of values are nil or false. Like this:

local success, response = pcall(function()
	assert(player.Character)
	assert(player.Character:FindFirstChild('Humanoid'))
	assert(player.Character:FindFirstChild('Humanoid').Health > 0)
	assert(player.Character:FindFirstChild('Humanoid').RootPart)
end)
if not success then return end

Assert is to error if a value is nil or false. Pcalls will detect errors, and if there is one, the success variable will be false. So, if not success, then return/stop the code!

Now, you don’t have to deal with a bunch of indents and ends on ends…

Hope this little tip helps clean up anyone’s code!

VegetationBush · November 11, 2022, 7:44pm

This will not work. You’re initializing the table with said values, and if it so happens that Character is nil, the script will error.

This is also even less efficient, as now there’s redundant operations being done added on to the already existing if statements.

commitblue · November 11, 2022, 8:23pm

Your better off ATLEAST using guard clauses or assert (with assert only if your fine with errors on falsey condition) but tables are less efficient.
guard clauses:

local z = true
local x = false
if not z then return end
if x then return end
print("i will print")

assert should be an easy to guess.

why are guard clauses faster and better? cause they directly return a falsey value ending the current scope (or function) directly.

CoderHusk · November 11, 2022, 8:23pm

Keep in mind that you changed your code, you have a return exit structure now. I honestly don’t see why people try and “fix” stuff that’s not even broken. There’s nothing wrong with having a bunch of if statements, especially considering Lua’s syntax makes it easy to find your encapsulation.

commitblue · November 11, 2022, 8:26pm

Having to look at an if bow is not so clean and not many people prefer it because when the bow becomes so big, your gonna have to keep scrolling horizontally to edit some conditions in those if blocks.
guard clauses? you dont need to scroll much. and it directly exits the current scope making it faster. HOWEVER, there is some cases where if bows are better than guard clauses.

CoderHusk · November 11, 2022, 8:28pm

Sure, but just know that your code is not the same as the other. Your guard clauses now won’t run anything after them now if they were true. You don’t necessarily get performance by doing this, you get performance with fundamental algorithms because anything that you could try to implement has been done better than anything you can do through the roblox api

commitblue · November 11, 2022, 8:29pm

thats exactly the point

GibusWielder · November 12, 2022, 3:22am

Alright, since the proposed solution didn’t work (as proven by TOP_Crundee123), I came up with another. It’s not as clean as the previous solution, but it’s still miles better than using a bunch of if statements.

SpaceBlacksmith · November 12, 2022, 9:07am

This thread shows that assert have worse performance than if statements:

commitblue · November 12, 2022, 9:54am

its still fast because he didnt supply the second argument or put anything to be evaluated in the second argument

BUT, pcalls are a bit expensive. pcall creates a new thread for a reason and its not rlly cheap. it should be Only used when nessecary.

guard clauses or if your fine with if bows are the best solution so far

round_edsquare · November 12, 2022, 4:35pm

Just to expand on what @commitblue is saying…

Guard clauses are a great solution for tidying up code and work well when combined with the or operator:

if not player.Character then return end
--assuming these are actually needed:
local humanoid, humanoidRootPart = player.Character:FindFirstChild("Humanoid"), player.Character:FindFirstChild("HumanoidRootPart")
if not humanoid or not humanoidRootPart then return end
--do stuff

A function with guard clauses (FilterInput) can even be used as a guard clause to help clean up the main code block (DoStuff):

local function FilterInput(inputData)
	if not (type(inputData) == "number") or inputData ~= inputData then return end
	if inputData % 1 ~= 0 or inputData > 3 or inputData < 1 then return end
	return true
end

local function DoStuff(inputData)
	if not FilterInput(inputData) then return end
	print(inputData, " is accepted")
end

DoStuff()
DoStuff("A")
DoStuff("1")
DoStuff(Instance.new("Part"))
DoStuff({10000})
DoStuff(function() inputData = 1 end)
DoStuff(-1)
DoStuff(0)
DoStuff(1/0)
DoStuff(1e1)
DoStuff(1.1)
DoStuff(math.huge)
DoStuff(2) --> Prints:  "2 is accepted"

For me, this increases readability in the filter as well as the main block of code. Bonus points for adding a bit of necessary abstraction.

MagmaBurnsV · November 12, 2022, 5:09pm

This is such an extreme case of overengineering. Never should you be intentionally erroring to control the logic of code.

Instead, you should be utilizing nillable variables and if-expressions to create more concise if-statements. For example, you could’ve done

local Character: Model? = ...
local Humanoid: Humanoid? = if Character then Character:FindFirstChildWhichIsA("Humanoid") else nil
local RootPart: BasePart? = if Humanoid then Humanoid.RootPart else nil

if Character and Humanoid and RootPart then -- Instances First
	if Humanoid.Health > 0 then -- Properties Second
		
	end
end

Which both is more structurally sound and also produces more efficient bytecode.

I would also discourage the use of guard-clauses because they end up confusing the Type-Checker and generally just makes code readability all the more confusing.

ducksandwifi · November 12, 2022, 7:34pm

Worth mentioning that FindFirstChild and its methods alike always return nil if it can’t find the X in the Y, no need for the else

For me personally, I use guard clauses if I expect the condition I’m checking to return a non-truey value most of the time.

assert is meant to be for debugging purposes, so I avoid that for condition-checking if I expect (or in a better word, guarantee) that it should return true 99.99% of the time (this doesn’t include checking if a Character exists or has a humanoid - I can’t guarantee that therefore I shouldn’t use assert!).

In extreme cases where for whatever reason I can’t just simply compare against a table, I use a lua-implementation of switch cases because they look more organised and have a better structure than a long if statement loops.

My conclusion however: It’s up to you if it couldn’t be put in a more optimised and readable form (this doesn’t including micro-optimisations).

vrs2210 · December 13, 2022, 5:56am

This is really bad, you’re making a useless function that increases the risk of no readability and lower performance if used.

paswa · December 13, 2022, 2:36pm

This is just worse looking. And still is heavy on performance. Just use and / or statements if it looks cluttered

round_edsquare · December 13, 2022, 3:18pm

tl;dr: It’s preference

The entire bottom half of the the code provided shows how useful the function is. All data received by the client needs to be filtered similar to this otherwise the code is highly exploitable. The function filters all data passed by a client and only allows the values 2 and 3, as an example. This is especially useful if it is inside a module script called by any script that needs to filter data received by the client, for code reusability.

I find this line to be extremely readable:
if not FilterInput(inputData) then return end

The code example was written a month ago, yet the code almost instantly reveals what it is doing. The main point behind the second function is to show abstraction. In that example, the function DoStuff does not need to know how the data is filtered, just whether it was filtered or not; this is also an example of functional programming. Sure, it can be said that two lines are not worth the abstraction, but the readability will decrease in the main function for every extra line of code needed to filter data. In other words, this method is scalable.

Yes everything has a cost, including calling a function. For completeness, here is a speed test comparison between running the original code and one without the secondary function 1000 times:

With Extra Function: 0.00005190001684240997
Without Extra Function: 0.00003460000152699649

This means the difference in time between the two, on average, is 0.0000000173 per call. If the server is suffering in performance from calling a function like this, then the problem is elsewhere. Lastly, this method is more popular than one might think. For example, these are lines 344-346 of ProfileService:

if continue_callback() ~= true then
	return
end

All that being said, it’s really just preference; I just went a little further to explain why I personally prefer to use this method. There’s really nothing wrong with using or not using guard clauses, as well as single-function architecture vs functional programming. Code it the best way that works for you and take approaches such as guard clauses as nothing more than a different way to accomplish the same task.