Point out the inefficienes in my code

Help and Feedback Code Review
#1

So basically, I’ve made this script that determines a point’s distance from a region of points.
See Example:

This is the main bit of the code the rest of the code is just pure mathematical solving (the functions being called)

while true do
	task.wait()
	for index, landPoint in	ipairs(LandPoints:GetChildren()) do
		landPoint.Color = Color3.new(1, 0, 0)
		if (ClosestPoint.Position-PointA.Position).Magnitude > (landPoint.Position-PointA.Position).Magnitude then
			ClosestPoint = landPoint
		end
	end
	
	local ClosePointNum = tonumber(string.split(ClosestPoint.Name,"t")[2])
	local LeftPoint
	local RightPoint
	
	if ClosePointNum == 1 then
		RightPoint = LandPoints:FindFirstChild("Point"..tostring(ClosePointNum+1))
		LeftPoint = LandPoints:FindFirstChild("Point"..tostring(#LandPoints:GetChildren()))
	elseif ClosePointNum == #LandPoints:GetChildren() then
		RightPoint = LandPoints:FindFirstChild("Point1")
		LeftPoint = LandPoints:FindFirstChild("Point"..tostring(ClosePointNum-1))
	else
		LeftPoint = LandPoints:FindFirstChild("Point"..tostring(ClosePointNum-1))
		RightPoint = LandPoints:FindFirstChild("Point"..tostring(ClosePointNum+1))
	end
	
	local PointDistance
	
	local RightPointTri = checkForRightAngle(SidesOfATriangle(ClosestPoint.Position, RightPoint.Position, PointA.Position))
	local LeftPointTri
	if RightPointTri then
		RightPoint.Color = Color3.new(0.458824, 1, 0.952941)
		local sideA, sideB, sideC = SidesOfATriangle(ClosestPoint.Position, RightPoint.Position, PointA.Position)
		PointDistance = calulateHeightOfTriangle(sideA, sideB, sideC)
	else
		local sideA, sideB, sideC = SidesOfATriangle(ClosestPoint.Position, LeftPoint.Position, PointA.Position)
		LeftPointTri = checkForRightAngle(sideA, sideB, sideC)
		if LeftPointTri then
			LeftPoint.Color = Color3.new(0.458824, 1, 0.952941)
			PointDistance = calulateHeightOfTriangle(sideA, sideB, sideC)
		end
	end
	
	if PointDistance == nil then
		PointDistance = (ClosestPoint.Position-PointA.Position).Magnitude
	end
	ClosestPoint.Color = Color3.new(0.129412, 1, 0.203922)
	
	print(math.round(PointDistance))
end
1 Like
#2

First of all, use runservice instead of while loop. Second of all, when you have an infinite loop, your goal is to do as much of the computation outside of that loop. Whatever’s in that loop runs every tick, so it needs to be as minimal as possible.

I see you’re calling GetChildren multiple times, this is computation that can be done outside of the loop and stored in a local variable.

I see you’re doing concat & split operations, which you want to avoid doing every tick, as they can be slow. I think you should sort your children alphabetically (if not already) and then just use the closeNumPoint as an index in the already computed array and do math.clamp(index - 1, 1, #arr) same for index + 1 to get right and left points. Simple math on numbers and array accessing is much faster than those string operations.

Hope this helps

2 Likes
#3

I will try and implement this when I get home.

1 Like
#4 
local function getPointDistance(TargetPointPosition, PointsDic)
	
	local ClosestPoint = PointsDic[1]
	local ClosestPointNum = 1
	for key, pointPos:Vector3 in PointsDic do
		if (pointPos-TargetPointPosition).Magnitude < (pointPos-ClosestPoint).Magnitude then
			ClosestPoint = pointPos
			ClosestPointNum = key
		end
	end
	local LeftPoint
	local RightPoint
	
	if ClosestPointNum == 1 then
		RightPoint = PointsDic[2]
		LeftPoint = PointsDic[#PointsDic]
	elseif ClosestPointNum == #PointsDic then
		RightPoint = PointsDic[1]
		LeftPoint = PointsDic[ClosestPointNum-1]
	else
		LeftPoint = PointsDic[ClosestPointNum-1]
		RightPoint = PointsDic[ClosestPointNum+1]
	end
	--print(ClosestPointNum, ClosestPoint, RightPoint, LeftPoint)
	local PointDistance
	
	local sideA, sideB, sideC = SidesOfATriangle(ClosestPoint, RightPoint, TargetPointPosition)
	local RightPointTri = checkForRightAngle(sideA, sideB, sideC)
	local LeftPointTri
	if RightPointTri then
		PointDistance = calulateHeightOfTriangle(sideA, sideB, sideC)
	else
		sideA, sideB, sideC = SidesOfATriangle(ClosestPoint, LeftPoint, TargetPointPosition)
		LeftPointTri = checkForRightAngle(sideA, sideB, sideC)
		if LeftPointTri then
			PointDistance = calulateHeightOfTriangle(sideA, sideB, sideC)
		end
	end
	
	if PointDistance == nil then
		PointDistance = (ClosestPoint-TargetPointPosition).Magnitude
	end
	
	return PointDistance
end

So here is the revised coded I made it into a function because of the usage I’ll be using it for.

2 Likes
#5

You should separate it into smaller functions, like getSomethingValue or calculatePointDistance, this will make it a lot more redable

There is a principle that tells the code should be shorter for the more complex it is, this might help you, because if something is very complex, you should try to split it into it’s own function

For instance, in your case there are many for loop math calculations and elseif block, you can simply put those in separate function, and make it a lot cleaner

You can turn getting pointPos into another function, that will base off table you’ll provide and other variables

This way your code should look a lot better

2 Likes
#6

Concatenation is only slow with large strings, "Point"..1 is not a large string.

#7

Yes but indexing and numeric operations are still faster than concantenation.

#8

Yea I have to agree with you on that one

#9

Rn Ive changed my use case so it’s only a dictionary of vectors