Better sword script code review

Help and Feedback Code Review
#1

I wanted to improve my Roblox scripting skills, and the first thing that I have done after learning all the basic concepts is inspecting scripts/codes in the Toolbox. The first item that I have inspected its code is the ordinary and normal Roblox sword tool.

When I inspected the code, I notice some problems:

  • The code itself is poorly organised, meaning the file structure of the code is really messy which makes the code less readable.

    image
    image530×638 46.5 KB

  • There are many unnecessary checks for the code, and I believe these checks are meant to ensure the sword works properly under certain circumstances.

function CheckIfAlive()
	return (((Player and Player.Parent and Character and Character.Parent and Humanoid and Humanoid.Parent and Humanoid.Health > 0 and Torso and Torso.Parent) and true) or false)
end

	if not Tool.Enabled or not ToolEquipped or not CheckIfAlive() then
		return
	end

function Blow(Hit)
	if not Hit or not Hit.Parent or not CheckIfAlive() or not ToolEquipped then
		return
	end
	local RightArm = Character:FindFirstChild("Right Arm") or Character:FindFirstChild("RightHand")
	if not RightArm then
		return
	end
	local RightGrip = RightArm:FindFirstChild("RightGrip")
	if not RightGrip or (RightGrip.Part0 ~= Handle and RightGrip.Part1 ~= Handle) then
		return
	end
	local character = Hit.Parent
	if character == Character then
		return
	end
	local humanoid = character:FindFirstChildOfClass("Humanoid")
	if not humanoid or humanoid.Health == 0 then
		return
	end
	local player = Players:GetPlayerFromCharacter(character)
	if player and (player == Player or IsTeamMate(Player, player)) then
		return
	end
	UntagHumanoid(humanoid)
	TagHumanoid(humanoid, Player)
	humanoid:TakeDamage(Damage)	
end
  • Poor management on performance as there is unnecessary memory leaks, mainly caused by events not being disconnected when they are no longer used in the code.
  • The sword does damage when the player doesn’t activate (click) it. Also, the sword does additional damage to other targets even if the targets were hit by the sword already.

So after figuring out all of the problem addressed above, I have decided to test my Roblox scripting skills by creating a brand new script which is the exact same as the old script but better, which solves all the problems mentioned above. First, I have made a simple flowchart which describes the whole sword system:

test.drawio
test.drawio851×621 70.3 KB

After that, I have coded the sword system by following the flowchart via creating one server script:

local Players = game:GetService("Players")

local DAMAGE_TABLE = {
	SLASH_DAMAGE = 10,
	LUNGE_DAMAGE = 50
}
local ANIMATIONS_TABLE = {
	Slash = "rbxassetid://16935567961",
	Lunge = "rbxassetid://16938434031"
}

local lastAttacked = 0
local attackCooldown = false
local handleTouchedDebounce = {}

local swordTool = script.Parent
local swordHandle = swordTool.Handle
local attackAnim = swordTool.AttackAnim

local character = swordTool.Parent.Parent.Character
local humanoid = character:WaitForChild("Humanoid")
local animator : Animator = humanoid:WaitForChild("Animator")

local swordActivatedConnection : RBXScriptConnection = nil
local playAnimationConnection : RBXScriptConnection = nil

local function playAnimation(animationType)
	if animationType == "Slash" then
		attackAnim.AnimationId = ANIMATIONS_TABLE.Slash
	elseif animationType == "Lunge" then
		attackAnim.AnimationId = ANIMATIONS_TABLE.Lunge
	else
		error("function playAnimation received an unknown type: "..animationType)
	end
	
	local animTracks : AnimationTrack = animator:LoadAnimation(attackAnim)
	animTracks:Play()
	
	return animTracks
end

local function onActivated()
	if not attackCooldown then
		attackCooldown = true
		
		local touchedEvent : RBXScriptConnection = nil
		
		local attackAnimTracks : AnimationTrack = nil
		local attackType
		local targetDamaged
		local damage
		
		local currentTick = tick()
		local timeDifference = currentTick - lastAttacked
		
		if timeDifference > 2 then -- SLASH
			attackType = "Slash"
			attackAnimTracks = playAnimation("Slash")
			damage = DAMAGE_TABLE.SLASH_DAMAGE
		else -- LUNGE
			attackType = "Lunge"
			attackAnimTracks = playAnimation("Lunge")
			damage = DAMAGE_TABLE.LUNGE_DAMAGE
		end
		
		touchedEvent = swordHandle.Touched:Connect(function(hit)
			local attackTarget
			attackTarget = hit.Parent
			
			if attackTarget.ClassName == "Model" and (not handleTouchedDebounce[attackTarget]) then
				handleTouchedDebounce[attackTarget] = true
				
				local humanoid = attackTarget:FindFirstChildWhichIsA("Humanoid")
				
				if humanoid and not Players:GetPlayerFromCharacter(attackTarget) then
					humanoid:TakeDamage(damage)
					targetDamaged = true
				end
			end
			
			if targetDamaged then
				attackAnimTracks.Ended:Wait()
				handleTouchedDebounce[attackTarget] = nil
			end
		end)
		
		playAnimationConnection = attackAnimTracks:GetMarkerReachedSignal(attackType):Connect(function()
			local attackSound : Sound = swordHandle[attackType]
			attackSound:Play()
			attackSound.Ended:Wait()
			playAnimationConnection:Disconnect()
		end)
		
		attackAnimTracks.Ended:Wait()
		
		touchedEvent:Disconnect()
		lastAttacked = currentTick
		attackCooldown = false
	end
end

local function onUnequipped()
	if swordActivatedConnection ~= nil then
		swordActivatedConnection:Disconnect()
	end
end

local function onEquipped()
	if humanoid.Health ~= 0 then
		swordHandle.Unsheath:Play()
		swordActivatedConnection = swordTool.Activated:Connect(onActivated)
	end
end

swordTool.Equipped:Connect(onEquipped)
swordTool.Unequipped:Connect(onUnequipped)

After I have conducted some testing, the code works well. Here’s why I believe my code works better than the old script:

  • The sword now only does damage ONCE to each different target when the sword handle touches different targets. After the sword attacking animation is complete, the sword can later deal damage to those different targets again, without dealing additional unnecessary damage in one single hit which could result an instant death of the target.
  • The sword is better in terms of performance, as the script makes sure that, when the sword is unequipped (or not equipped), it disconnects the Activated event which is responsible for dealing damage to targets.
  • The script/code itself is more organised, which increases readability.

Here’s a video demonstrating how the new sword works.

However, I believe there are still some rooms for improvement:

  • Is my code organised? Where should I improve in order to improve its readability?
  • Is my script optimised? In which part of my script should I pay attention and try to optimise?
  • What are the other disadvantages that my script can’t do? How should I approach to solving it?
  • Are there any other suggestions which I can add in my script?
  • Is there any other decomposition diagram maker or flowchart maker for me to create flowcharts like the one above?

I hope my script can be thoroughly analysed and interpreted by anyone for feedbacks and constructive criticisms. Should there be any questions regarding the script itself, please let me know!

Thanks.

3 Likes