Can this code be optimized/improved? Organized better or anything

Help and Feedback Code Review
#1

Hello, I have 3 modules, 2 pretty long and somewhat messy but im not sure what would make them look better. Just want some other opinions, and some opinions on if certain things can be improved/made better.

CharacterClass 
local player = game:GetService("Players").LocalPlayer
local input_service = game:GetService("UserInputService")
local rs = game:GetService("ReplicatedStorage")
local run_time = game:GetService("RunService")
local debris = game:GetService("Debris")

local shared = rs.shared
local network = require(shared.network)
local controls = require(script.Parent.CharacterControls)
local kaiju_stats = require(shared.kaiju_stats)
local animator = require(script.Parent.Animator)
local combos = require(shared.Combos)
local mover_util = require(script.MoverUtil)

local camera = workspace.CurrentCamera

local active_character = nil -- garbage collecting.... bleh.

local module = {
	current = nil,
}

local function lerp(a, b, t)
	return a + (b - a) * t
end

local function SetupDebuggingSpeeds(character: Model, speeds: {number}, fn: (type: string, new: number) -> ())
	if not run_time:IsStudio() then
		return
	end
	
	character:SetAttribute("Walk", speeds.Walk)
	character:SetAttribute("Sprint", speeds.Sprint)
	
	character:GetAttributeChangedSignal("Walk"):Connect(function()
		fn("1", character:GetAttribute("Walk"))
	end)
	
	character:GetAttributeChangedSignal("Sprint"):Connect(function()
		fn("2", character:GetAttribute("Sprint"))
	end)
end

function module:Init()
	network:GetRemote("Spawn"):FireServer()
	
	player.CharacterAdded:Connect(function()
		self.current = self.new()
	end)
	
	player.CharacterRemoving:Connect(function()
		if self.current then
			self.current:Destroy()
		end
	end)
end

function module.new()
	if active_character then
		active_character:Destroy()
	end
	print("Constructing character...")

	local character = player.Character :: Model
	local humanoid_root_part = character:WaitForChild("HumanoidRootPart", math.huge) :: BasePart
	local humanoid = character:WaitForChild("Humanoid") :: Humanoid

	local stats = kaiju_stats[character.Name:lower()]
	local WALK_SPEED = stats.Config.WalkSpeed
	local SPRINT_SPEED = stats.Config.SprintSpeed
	
	local current_speed = 0
	local goal_speed = WALK_SPEED
	local is_sprinting = false
	
	SetupDebuggingSpeeds(character, {Walk = WALK_SPEED, Sprint = SPRINT_SPEED}, function(type, speed)
		if type == "1" then
			WALK_SPEED = speed
		else
			SPRINT_SPEED = speed
		end
	end)
	
	camera.CameraSubject = character:WaitForChild("CameraPart")
	humanoid.WalkSpeed = 0
	humanoid.AutoRotate = true
	humanoid.JumpPower = 0
	humanoid.UseJumpPower = true
	character:SetAttribute("Sprinting", false)
	character:SetAttribute("Attacking", false)
	
	character:GetAttributeChangedSignal("Sprinting"):Connect(function()
		is_sprinting = character:GetAttribute("Sprinting")
	end)
	
	controls:Construct()
	animator:Load(stats.Animations)
	active_character = {
		connections = {},
	}
	
	table.insert(active_character.connections, run_time.Heartbeat:Connect(function(dt)
		active_character:Update(dt)
	end))
	
	table.insert(active_character.connections, network:GetRemote("Flinch"):Connect(function()
		animator:Play("Flinch")
	end))
	
	table.insert(active_character, network:GetRemote("Knockback"):Connect(function(...)
		active_character:Push(...)
	end))
	
	function active_character:Update(delta_time)
		local humanoid_move_direction = humanoid.MoveDirection
		local is_moving = humanoid_move_direction do
			if is_moving == Vector3.zero then
				is_moving = false
			else
				is_moving = true
			end
		end
		
		local speed_ratio = 1
		
		if character:GetAttribute("Attacking") or self.KnockingBack or self.Stunned then
			if humanoid.AutoRotate then
				humanoid.AutoRotate = false
			end
			
			local new_goal = goal_speed / 2
			if current_speed <= new_goal then
				humanoid.WalkSpeed = current_speed
			else
				humanoid.WalkSpeed = new_goal
			end
		else
			if not humanoid.AutoRotate then
				humanoid.AutoRotate = true
			end
			
			local speed_difference = goal_speed - current_speed
			if speed_difference < 0 then
				speed_difference *= -1
			end
			if speed_difference < 0.1 then
				current_speed = goal_speed
			elseif speed_difference > 0.1 then
				-- math.clamp(3 * delta_time, 0.05, 0.2)
				-- DEFAULT
				local x = math.clamp(1 * delta_time, 0.05, 0.2)
				current_speed = lerp(current_speed, goal_speed, x)
			end

			speed_ratio = current_speed / goal_speed
			humanoid.WalkSpeed = current_speed
		end
		
		if is_moving then
			goal_speed = if is_sprinting then SPRINT_SPEED else WALK_SPEED
			
			if character:GetAttribute("Attacking") or self.KnockingBack or self.Stunned then
				local root_cframe = humanoid_root_part.CFrame
				local root_rotation = root_cframe.Rotation
				local rotation_goal = CFrame.new(humanoid_root_part.Position + humanoid_move_direction)
				humanoid_root_part:PivotTo(root_cframe:Lerp(CFrame.new(root_cframe.Position, rotation_goal.Position), 1 * delta_time))
			end
			
			-- 0.1 by default
			
			if is_sprinting then
				animator:Play("Sprint"):AdjustSpeed(speed_ratio)
			else
				animator:Play("Walk"):AdjustSpeed(speed_ratio)
			end
		else
			goal_speed = 0
			animator:Play("Idle")
		end
	end

	function active_character:Destroy()
		print("Deconstructing character...")
		pcall(function()
			for _, connection: RBXScriptConnection in self.connections do
				connection:Disconnect()
			end
			table.clear(self.connections)
		end)
		
		animator:Deload()
		combos:Reset("All")
		module.current = nil
		controls:Deconstruct()
		table.freeze(self)
		active_character = nil
	end
	
	function active_character:Push(direction: Vector3 | string?, strength: number, length: number)
		self.KnockingBack = true
		
		if typeof(direction) == "string" then
			direction = direction:lower()
			if direction == "backwards" then
				direction = -humanoid_root_part.CFrame.LookVector * strength
			end
		end
				
		local a1 = Instance.new("Attachment")
		a1.Name = "K_A_1"
		a1.Parent = humanoid_root_part

		local linear_velocity = Instance.new("LinearVelocity")
		local limit = 10000000
		linear_velocity.Attachment0 = a1
		--linear_velocity.RelativeTo = Enum.ActuatorRelativeTo.Attachment0
		linear_velocity.ForceLimitMode = Enum.ForceLimitMode.PerAxis
		linear_velocity.MaxForce = Vector3.new(limit, 0, limit)
		linear_velocity.VectorVelocity = direction
		linear_velocity.Parent =  humanoid_root_part
		
		debris:AddItem(linear_velocity, length or 0.5)
		task.delay(length or 0.5, function()
			self.KnockingBack = false
		end)
	end
	
	return active_character
end

function module:GetActive()
	return active_character
end

return module
CharacterControls

Keep in mind CreateHitbox1 is old, I just didn’t want to delete the function entirely as I was/am still messing around with hitboxes.

--!strict
local player = game:GetService("Players").LocalPlayer
local input_service = game:GetService("UserInputService")
local rs = game:GetService("ReplicatedStorage")
local run_time = game:GetService("RunService")
local debris = game:GetService("Debris")

local shared = rs.shared
local network = require(shared.network)
local kaiju_stats = require(shared.kaiju_stats)
local cooldowns = require(shared.Cooldowns)
local hitbox_handler = require(script.MuchachoHitbox)
local animator = require(script.Parent.Animator)
local combos = require(shared.Combos)
local ingame = require(script.Parent.Ingame)
--local hud = require(script.Parent.HUD)

local hitboxes = rs.shared.hitboxes

local last_attack = 0
local active_cooldown_time = 0
local last_attack_anim = nil :: AnimationTrack?

local module = {
	constructed = false,
	connections = {}
}

local function CanAttack(attack)
	return cooldowns:CheckCooldown(attack)
end

local function GetCharacter(): Model?
	return player.Character
end

local function GetHumanoid(): Humanoid?
	local character = GetCharacter()
	local humanoid = character and character:WaitForChild("Humanoid") :: Humanoid
	return humanoid
end

local function GetHitbox(name: string): Part?
	local character = GetCharacter()
	if not character then
		return nil
	end
	return hitboxes[character.Name:lower()]:FindFirstChild(name)
end

local function CreateHitbox(origin_hitbox: Part, offset: Vector3, visualize: boolean?): number?
	local params = OverlapParams.new()
	local blacklist = {GetCharacter()}
	params.FilterDescendantsInstances = blacklist
	params.FilterType = Enum.RaycastFilterType.Exclude

	local character = GetCharacter()
	if not character then -- makes type checking happy..
		return 0
	end
	local humanoid_root_part = character:WaitForChild("HumanoidRootPart") :: Part
	local hitlist = {}
	
	local hitbox = nil
	local x, y, z = humanoid_root_part.CFrame:ToOrientation()
	local goal_cframe = humanoid_root_part.CFrame * CFrame.new(offset)
	if visualize then
		hitbox = origin_hitbox:Clone()
		hitbox.CFrame = goal_cframe
		hitbox.Parent = character
		debris:AddItem(hitbox, 1)
	end
	
	local touching = workspace:GetPartBoundsInBox(goal_cframe, origin_hitbox.Size, params)
	for _, v: Part in pairs(touching) do
		if v.Name == "HumanoidRootPart" or v.Transparency == 1 then
			continue
		end
		
		local parent = v.Parent :: Model
		local humanoid = parent:FindFirstChildWhichIsA("Humanoid")
		if humanoid then
			if table.find(hitlist, humanoid) then
				continue
			end
			table.insert(hitlist, humanoid)
			
			local highlight = Instance.new("Highlight")
			highlight.FillTransparency = 0.8
			highlight.Parent = v.Parent
			debris:AddItem(highlight, 0.3)
		end
	end
	
	if #hitlist > 0 then
		network:GetRemote("Hit"):FireServer(hitlist)
	end
	
	return #hitlist
end

local function CreateHitbox1(origin_hitbox: Part, offset, part: Part, lifetime: AnimationTrack): number?
	local params = OverlapParams.new()
	params.FilterDescendantsInstances = {GetCharacter()}
	params.FilterType = Enum.RaycastFilterType.Exclude

	local hitbox = hitbox_handler.CreateHitbox()
	hitbox.Size = origin_hitbox.Size
	hitbox.CFrame = part
	hitbox.Visualizer = false
	hitbox.Offset = CFrame.new(offset)
	hitbox.OverlapParams = params
	
	local hitcount = 0
	local hit_keyframe = false
	
	hitbox.Touched:Connect(function(hitpart, humanoid)
		--print("Hit", hitpart:GetFullName())
		hitcount += 1
		network:GetRemote("Hit"):FireServer(humanoid)
		
		local highlight = Instance.new("Highlight")
		highlight.FillTransparency = 0.8
		highlight.Parent = hitpart.Parent
		debris:AddItem(highlight, 0.3)
	end)

	hitbox:Start()
	--task.wait(lifetime - (lifetime * 0.2))
	lifetime:GetMarkerReachedSignal("Hit"):Wait()
	hitbox:Stop()
	return hitcount
end

local function LmbAttack()
	if cooldowns:CheckCooldown("Attack") then
		return
	end
	if cooldowns:CheckCooldown("LMB") then
		return
	end
	cooldowns:InfYield("LMB")
	cooldowns:InfYield("Attack")
	ingame:MassCooldown(true)
	
	local character = player.Character
	local combo = combos:Get("LMB")
	local stats = kaiju_stats[character.Name:lower()].Abilities.LMB[combo]
	local hitbox = GetHitbox(`m{combo}`)
	
	local this_attack = {
		combo = combo
	}
		
	if not hitbox then
		warn(`Attack #{combo} failed due to the part "{stats.Part}" not existing.`)
		combos:Reset("LMB")
		cooldowns:AddCooldown("LMB", 0.2)
		return
	end
	
	if character then
		character:SetAttribute("Attacking", true)
	end
	
	local current_anim = animator:Play(`M{combo}`)
	last_attack_anim = current_anim
	local length = current_anim.Length
	current_anim:GetMarkerReachedSignal("Hit"):Wait()
	
	--local hits = CreateHitbox(hitbox, stats.Offset, target_part, current_anim)
	local hits = CreateHitbox(hitbox, stats.Offset, true)
	local cooldown_time = if hits and hits > 0 then 0.2 else 1.5
	this_attack.hits = hits
	
	if hits and hits > 0 then
		local old_combo = combos:AddCombo("LMB", 3)
		if old_combo == 3 then
			cooldown_time = 1
		end
	else
		combos:ResetCombo("LMB")
	end
	
	if character then
		character:SetAttribute("Attacking", false)
	end
	--combos:Unfreeze("LMB")
	ingame:MassCooldown(false)
	for x = 1, 3 do
		ingame:UpdateIcon(`m{x}`, true, cooldown_time, function()
			if x ~= combos:Get("LMB") then
				ingame:UpdateIcon(`m{x}`, true)
			end
		end)
	end
	cooldowns:AddCooldown("Attack", 0.5)
	cooldowns:AddCooldown("LMB", cooldown_time)
end

local function Dash()
	if cooldowns:CheckCooldown("Dash") then
		return
	end
	
	cooldowns:AddCooldown("Dash", 0.3)
	
	local character = GetCharacter()
	if not character then
		return
	end
	
	local humanoid_root_part = character:WaitForChild("HumanoidRootPart") :: Part
	local part = Instance.new("Part")
	part.Size = Vector3.new(4,4,4)
	part.Transparency = 1
	part.CanCollide = false
	part.Anchored = true
	part.Position = humanoid_root_part.Position + (humanoid_root_part.CFrame.LookVector * 25)
	part.Parent = workspace
	local a1 = Instance.new("Attachment")
	a1.Parent = humanoid_root_part
	local a2 = a1:Clone()
	a2.Parent = part
	
	local line_force = Instance.new("LineForce")
	line_force.ApplyAtCenterOfMass = true
	line_force.Magnitude = 10000000
	line_force.Attachment0 = a1
	line_force.Attachment1 = a2
	line_force.Parent = humanoid_root_part
	debris:AddItem(a1, 0.1)
	debris:AddItem(part, 0.1)
	debris:AddItem(line_force, 0.1)
end

function module:Init()
	ingame:UpdateIcon("m2", true)
	ingame:UpdateIcon("m3", true)
end

function module:Construct()
	if self.constructed then
		return
	end
	print("Constructing character controls")
		
	self:Connect({
		input_service.InputBegan:Connect(function(input, gp)
			if gp then
				return
			end
			
			if input.KeyCode == Enum.KeyCode.LeftShift then
				local character = GetCharacter()
				if not character then
					return
				end
				
				character:SetAttribute("Sprinting", true)
				--hud:Sprint(true)
			elseif input.UserInputType == Enum.UserInputType.MouseButton1 then
				LmbAttack()
			--else
				--local fn = abilities[input.UserInputType]
				--local name = input.UserInputType.Name
				--if not fn then
				--	fn = abilities[input.KeyCode]
				--	name = input.KeyCode.Name
				--end
				
				--if fn and not cooldowns:CheckCooldown(name) then
				--	print(name)
				--	fn()
				--end
			elseif input.KeyCode == Enum.KeyCode.F then
				--Dash()
				network:GetRemote("Knockback"):FireServer()
			end
		end),
		
		input_service.InputEnded:Connect(function(input, gp)
			if input.KeyCode == Enum.KeyCode.LeftShift then
				local character = GetCharacter()
				if not character then
					return
				end

				character:SetAttribute("Sprinting", false)
				--hud:Sprint(false)
			end
			
			if gp then
				return
			end
		end)
	})
	
	self.constructed = true
end

function module:Deconstruct()
	if not self.constructed then
		return
	end
	print("Deconstructing character controls")
	self:Disconnect()
	self.constructed = false
end

function module:Connect(list: {RBXScriptConnection})
	for _, connection in list do
		table.insert(self.connections, connection)
	end
end

function module:Disconnect()
	for _, connection: RBXScriptConnection in self.connections do
		connection:Disconnect()
	end
	table.clear(self.connections)
end

return module
Animator 
local animations = { }
local animationInterval = 0.4

local loaded = false
local onLoaded = Instance.new("BindableEvent")

local animationIgnoreList = {
	"M1", "M2", "M3", "Flinch"
}
local already_warned_for = {}
local ignore_list_animations = {}

local Animator = {}

local function StopListedAnimations(animName)
	for name, a in pairs(animations) do
		if not a.Stop then continue end
		if name == animName then continue end

		if animations[animName] and not animations[animName].Stop then
			if name == "Idle" then
				continue
			end
		end
		a.Anim:Stop(animationInterval)
	end
end

local function Get(self, animName: string)
	local anim = animations[animName]
	return anim and anim.Anim or nil
end

local function Play(self, animName, dontStop, dontTween): AnimationTrack
	if not loaded then return warn("Animator not loaded! Please load the animator before accessing methods.") end

	if not animations[animName] then
		if not already_warned_for[animName] then
			local warning = string.format(
				"[!] Failed to find %s in animation cache.",
				animName
			)
			warn(warning)
			already_warned_for[animName] = true
		end
		StopListedAnimations("")
		return
	end

	if animations[animName].Anim.IsPlaying then
		if table.find(animationIgnoreList, animName) then
			for _, ignore_list_animation in pairs(ignore_list_animations) do
				ignore_list_animation.Anim:Stop()
			end
			animations[animName].Anim:Play(.4)
		end
		
		return animations[animName].Anim
	end

	if table.find(animationIgnoreList, animName) then
		dontStop = true
		dontTween = true
		for _, ignore_list_animation in pairs(ignore_list_animations) do
			ignore_list_animation.Anim:Stop()
		end
	end

	if not dontStop then
		StopListedAnimations(animName)
	end

	if dontTween then
		animations[animName].Anim:Play()
	else
		animations[animName].Anim:Play(animationInterval)
	end
	return animations[animName].Anim
end

local function Stop(self, animName, dontTween)
	if not loaded then return warn("Animator not loaded! Please load the animator before accessing methods.") end

	if not animations[animName] then
		local warning = string.format(
			"[!] Failed to find %s in animation cache.",
			animName
		)
		warn(warning)
		return
	end

	if not animations[animName].Anim.IsPlaying then
		return
	end

	if dontTween then
		animations[animName].Anim:Stop()
	else
		animations[animName].Anim:Stop(animationInterval)
	end
end

local function Load(self, statAnimations, contentProvider)
	if loaded then return warn("Animator already loaded!") end
	loaded = true

	local character = game.Players.LocalPlayer.Character
	local humanoid: Humanoid = character:WaitForChild("Humanoid")
	local animator = humanoid.Animator

	for animationName, animationId in pairs(statAnimations) do
		if animationId == "rbxassetid://0" or animationId == "" or animationId == 0 then
			continue
		end

		local animation = Instance.new("Animation")
		animation.AnimationId = "rbxassetid://"..tostring(animationId)

		local loaded = animator:LoadAnimation(animation)
		local animName = animationName

		if table.find(animationIgnoreList, animName) then
			animations[animName] = {
				Anim = loaded;
				Length = loaded.Length;
				Stop = false;
			}
			ignore_list_animations[animName] = {
				Anim = loaded;
				Length = loaded.Length;
				Stop = false;
			}
		else
			animations[animName] = {
				Anim = loaded;
				Length = loaded.Length;
				Stop = true;
			}
		end

		animation:Destroy()
	end
end

local function Deload(self)
	loaded = false
	table.clear(already_warned_for)
	table.clear(ignore_list_animations)
	table.clear(animations)
end

local function UntilLoaded(self)
	if loaded then return end
	onLoaded.Event:Wait()
end

Animator.Play = Play
Animator.Load = Load
Animator.Stop = Stop
Animator.Await = UntilLoaded
Animator.Deload = Deload
Animator.Get = Get

return Animator

Sorry for such long code lol, I’ve been working on this project for 2 days now, I’ve made some pretty good progress. Just want to get some more opinions on it before moving further and adding more abilities.
The animator is one I made a few months ago, so I expect that one to be the worst of them all, with a few changes and whatnot throughout time.

Thank you for your time!

#2

One thing that could help is adding comments to explain why certain code is there, as well as more useful variable names sometimes. For example while it’s clear CreateHitbox1 is related to hitboxes and a1 is an attachment, it isn’t clear what their purpose is.

1 Like