Which option is better?

Help and Feedback Code Review
#1

Which one looks… better?

Coin.Touched:Connect(function(Part)
	local Char = Part:FindFirstAncestorOfClass("Model")
	if Char then
		local Player = Players:GetPlayerFromCharacter(Char)
		if Player then
			local Data = shared[Player.UserId]
			if Data then
				Data[2] += Value[Type]
				Coin:Destroy()
			end
		end
	end
end)

Or:

Coin.Touched:Connect(function(Part)
	local Char = Part:FindFirstAncestorOfClass("Model")
	if not Char then return end
	local Player = Players:GetPlayerFromCharacter(Char)
	if not Player then return end
	local Data = shared[Player]
	if not Data then return end
	Data[2] += Value[Type]
	Coin:Destroy()
end)
1 Like
#2

Instead of writing the conditions that needs to be met 1 line at a time, you can merge them with and/or. For example:

	local Char = Part:FindFirstAncestorOfClass("Model")
	local Player = Players:GetPlayerFromCharacter(Char)
    if Char and Player then
       --code...
    end
#3

But the problem is, if Char is nil, then there is, at best, an unnecessary call to GetPlayerFromCharacter and at worst, an error, if that function does not accept nil values.

#4

One looks so much better. It’s a lot more clean and readable.

#5

Idrk, at the end of the day this is for readability. There’s no perfect standard for organised code - that’s something you decide.

#6

Lowkey I use #2 except I add extra spaces between each variable check. So my scripts look like:

coin.Touched:Connect(function(part)
	local char = part:FindFirstAncestorOfClass("Model")
	if not char then return end

	local player = Players:GetPlayerFromCharacter(char)
	if not player then return end

	local data = shared[player]
	if not data then return end

	data[2] += value[t]
	coin:Destroy()
end)

Which might take up more space, but it’s so easy to skim through and understand.

1 Like
#7

You can do it like this to avoid that:

local Char = Part:FindFirstAncestorOfClass("Model")
local Player = Char and Players:GetPlayerFromCharacter(Char)
local Data = Player and shared[Player]
if not Data then return end
--code
2 Likes
#8

The second one is cleaner and more efficient because it uses early returns.

#9

the 2nd one looks better, but you can add some empty lines to make it easier to read

Coin.Touched:Connect(function(Part)
	local Char = Part:FindFirstAncestorOfClass("Model")
	if not Char then return end
	local Player = Players:GetPlayerFromCharacter(Char)
	if not Player then return end
	local Data = shared[Player]
	if not Data then return end
	Data[2] += Value[Type]
	Coin:Destroy()
end)

vs

Coin.Touched:Connect(function(Part)
	local Char = Part:FindFirstAncestorOfClass("Model")
	if not Char then return end
	
	local Player = Players:GetPlayerFromCharacter(Char)
	if not Player then return end
	
	local Data = shared[Player]
	if not Data then return end
	
	Data[2] += Value[Type]
	Coin:Destroy()
end)

but what Player has suggested is more readable than both of them