Feedback on code organization

Help and Feedback Code Review
1

Hello DevForum users!

So i am making a lerp library, i am not such a professional but i wanna learn.
Anyway, theres a part of the code that uses a RunService so i can optimize the loops a lot instead of doing a loop per lerp.

I create this post because it looks, kinda bad, to be honest, it works, but feels like unorganized.
Any feedback would be awesome!

RunService.Heartbeat:Connect(function(deltaTime: number)
	-- Get all lerps
	for index, self in Lerps do
		self.Time += deltaTime
		
		
		-- Add delta to lerp time
		
		local AlphaGoal = self.Alpha
		local alpha = self.Time / self.Info.Duration -- Get alpha
		
		
		
		if alpha >= AlphaGoal then
			self.DTime += deltaTime
			-- If alpha goal completed
			if self.Info.DelayTime >= 0 then
				if self.DTime < self.Info.DelayTime then continue end
			end
			
			-- Reset values
			ResetLerp(self)
			self.Completed:Fire()
			
			-- Verify loop
			if self.Info.RepeatCount <= -1 then
				if self.Info.Reverses then
					-- If reverses then set goal to starting
					self.Reverse = not self.Reverse
					if not self.Reverse then
						self.Object.CFrame = self.Starting
					else
						self.Object.CFrame = self.Value
					end
				else
					self.Object.CFrame = self.Starting
				end
				self:Play(self.Alpha)
			else
				self.Object.CFrame = self.Value
				self.Alpha = 0
			end
			table.remove(Lerps,index)
		else
			self:Play(alpha)
			self.DTime = 0
		end
	end
end)
2

Besides the many nestings, there’s also if self.Info.RepeatCount <= -1 then.

What’s -1? You have alphaGoal, which is a self documented part. But it’s not the same with self.info.delayTime you can make it more self documented having it as a variable and naming it delayTime and 0 you commented as completed so then self.Info.DelayTime >= 0 is gonna be DelayTime >= completed. See the difference? Much more readable and no code comments needed for that.

What’s self documented code? https://en.wikipedia.org/wiki/Self-documenting_code

2 Likes
3

The repeat count -1 is because in tween service works the same, just because of that hahaha.
Then the variable thing, i dont know, i just really just used self.
Then, the delay time, is to verify if the delayTime is more than 0 if thats the case start the delay, not if its completed or not.

Thank you for the feedback, is there any way to make less nested?

4

basically, self.Object.CFrame can be a variable called cframe and for the value that you’re checking, store it in a variable and call it what it is. For completion, pause, etc:

local cframe = self.Object.CFrame

local Starting = self.Starting
local Value = self.Value
local Reverse = self.Reverse

if not Reverse then
    cframe = Starting
else
    cframe = Value
end
1 Like
5

there’s a major logical error in your code

take this code for example

local x = {2, 4, 6, 7, 8, 9, 10, 11, 13, 14, 16, 17, 18}
for k, v in ipairs(x) do
	if (v % 2 == 0) then
		table.remove(x, k)
	end
end

print(x)

you’d expect it to remove every number divisible by 2 in the array
yet printing it yields:

image
image256×146 1.91 KB

this is because table.remove shifts the table but doesn’t shift the iterator in the for loop.

here’s a fix taken directly from my 1-life hardcore desert-survival game neniadea

function rvTable.remove(array, condition, onDestroy)
	local removeIndices = {}
	for k, v in ipairs(array) do
		if (condition(k, v)) then
			table.insert(removeIndices, k)
		end
	end

	-- remove
	if (next(removeIndices)) then
		for v = #removeIndices, 1, -1 do
			if (onDestroy) then
				onDestroy(removeIndices[v], array[removeIndices[v]])
			end
			table.remove(array, removeIndices[v])
		end
	end
end

some of my code’s cruddy but it solves the problem effectively

other than this, your codes good. theres a few things i’d change. stay consistent with the casing firstly. personally i’d use lowercase names instead of uppercase, so self.Object to self.object. also i prefer whatever casing thisCase is

for self.completed i’d change its name to OnCompletion, because completed signifies a boolean to me

self.Reverse = not self.Reverse
					if not self.Reverse then
						self.Object.CFrame = self.Starting
					else
						self.Object.CFrame = self.Value
					end

im not a fan of this segment. i’d rather have one condition not two. to ensure theres no bugs or whatever. also it would make this segment iin particular effortless to understand.

name self.DTime better (self.DeltaTime? self.AccumulatedDeltaTime?), same with self.Info. i’d probably change self.Reverse to self.shouldReverse

if you’re using a bindableevent for self.Completed i’d change that to a custom signal. roblox deprecates stuff left and right (even though bindable events have been iin the engine for years). plus a custom signal ensures your code works how you want it to. freedom.

personally i wouldnt use for index, self in Lerps do because i stick to ipairs or pairs generally.

but to sum up, consider changing your naming convention, fix that major bug that occurs (it occurs when 2 table values are completed at the same time and their indices are not the end of the table), and stray from robloxs stuff