Which option is better?

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)
4 Likes

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

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.

2 Likes

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

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

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.

5 Likes

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
6 Likes

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

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

Inversion is pretty usefull, as it reduce nesting, but what you really should think about is code redability, simpliest methood to do that is to separate your script by empty lines + add scoping to return statements

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)

EDIT: Also you shouldn’t use global tables, try using module scripts as a way of storing data, because they are better with performance and organization

Another thing is that you probably have hard-coded variable, Data[2] can’t have other value than that, maybe try naming variable and using it as costant instead of magic number?

1 Like