Improvement Guides needed for Sword Infrustructure

FerbZides · November 27, 2020, 7:47am

I just needed feedback for this code and the explorer properties because if you think it’s not good then ill be glad to rewrite it!

Server Code:

-- // Services \\ --

local RunService = game:GetService("RunService")

-- // Variables \\ --

local Tool = script.Parent.Parent.Parent.Parent
local Events = Tool.Events

-- // Events \\ --

local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock

-- // Functions \\ --
local function Yield(s)
	local t = tick()
	while tick()-t < s do RunService.Stepped:wait() end
	return tick()-t
end

Tool.Equipped:Connect(function()
	-- Make Motor 6D {first we need the player's character}
	local Character = script.Parent.Parent.Parent.Parent.Parent
	local Player = game.Players:GetPlayerFromCharacter(Character)
	local RightHand = Character.RightHand
	
	-- Creation of Motor6D
	local Motor6D = Instance.new("Motor6D")
	Motor6D.Name = "Motor6D"
	Motor6D.Parent = RightHand
	
	Motor6D.Part0 = RightHand
	Motor6D.Part1 = Tool.Sword
	
	PLAY_ANIMATION_EVENT:FireClient(Player, "Hold")
	CHANGE_MOUSE_EVENT:FireClient(Player, "WeaponMouseIcon")
	SHIFTLOCK_EVENT:FireClient(Player, "Enable")
	
end)

Tool.Unequipped:Connect(function()
	-- Remove Motor6D
	local Player = script.Parent.Parent.Parent.Parent.Parent.Parent
	local Character = Player.Character
	local RightHand = Character.RightHand
	
	-- Removal of MOtor6D
	local Motor6D = RightHand.Motor6D
	
	Motor6D.Part0 = nil
	Motor6D.Part1 = nil
	
	Motor6D:Destroy()
	
	PLAY_ANIMATION_EVENT:FireClient(Player, "StopHold")
	CHANGE_MOUSE_EVENT:FireClient(Player, "DefaultMouseIcon")
	SHIFTLOCK_EVENT:FireClient(Player, "Disable")
	
end)

Tool.Activated:Connect(function()
	-- Set Debounce to true then after a few milliseconds then to false
	local Player = script.Parent.Parent.Parent.Parent.Parent
	local Values = Tool.Values
	local SwordDebounce = Values.SwingDelay
	
	if SwordDebounce == false then
		SwordDebounce = true
		
		PLAY_ANIMATION_EVENT:FireClient(Player, "Swing")
		
		Yield(.5)
		SwordDebounce = false
	end
end)

Client Code:

-- // Services \\ --
local UIS = game:GetService("UserInputService")

-- // Variables \\ --
local Tool = script.Parent.Parent.Parent.Parent
local Events = Tool:WaitForChild("Events")

local Player = game.Players.LocalPlayer
local Character = Player.Character or Player.CharacterAdded:wait()
local Humanoid = Character.Humanoid

-- // For Shiftlock \\ --
local PlayerScripts = Player.PlayerScripts
local Values = PlayerScripts.Values
local IsHoldingTool = Values.IsHoldingTool

-- // For UI \\ --
local PlayerGui = Player.PlayerGui
local WeaponMouseIcon = PlayerGui.WeaponMouseIcon

-- // Events \\ --
local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock

-- // Animation Tracks \\ --
local AnimationsFolder = Tool.Animations
local SWING_ANIMATION = AnimationsFolder.Swing
local HOLD_ANIMATION = AnimationsFolder.Hold

PLAY_ANIMATION_EVENT.OnClientEvent:Connect(function(AnimationName)
	if AnimationName == "Hold" then
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)
		
		-- Play it bruh lol
		print("working")
		HOLD_ANIMATION_TRACK:Play()
		HOLD_ANIMATION_TRACK:AdjustWeight(1, .1)
	elseif AnimationName == "Swing" then
		local SWING_ANIMATION_TRACK = Humanoid:LoadAnimation(SWING_ANIMATION)
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)
		
		-- Play it bruh lol
		print("working")
		HOLD_ANIMATION_TRACK:Stop()
		SWING_ANIMATION_TRACK:Play()
	elseif AnimationName == "StopHold" then
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)

		-- Play it bruh lol
		print("working")
		HOLD_ANIMATION_TRACK:Stop()
	end
end)

CHANGE_MOUSE_EVENT.OnClientEvent:Connect(function(MouseIcon)
	if MouseIcon == "WeaponMouseIcon" then
		UIS.MouseIconEnabled = false
		WeaponMouseIcon.Enabled = true
	elseif MouseIcon == "DefaultMouseIcon" then
		UIS.MouseIconEnabled = true
		WeaponMouseIcon.Enabled = false
	end
end)

SHIFTLOCK_EVENT.OnClientEvent:Connect(function(Response)
	if Response == "Enable" then
		IsHoldingTool.Value = true
	elseif Response == "Disable" then
		IsHoldingTool.Value = false
	end
end)

Infrustructure:

Feedback is appreciated, Thanks!

hamsterloverboy3 · November 27, 2020, 9:09am

I’ve put my review in the form of comments of your original code. I’ve also removed all of your comments and changed some spacing. I strongly believe in having no comments in your code especially when it’s obvious what is going on. The code/style should be self documenting.

I am not a fan at all of the parent soup.

For the server code the main flaw is how you are fetching the player, character and motor6d. You should cache the results of these instances from when you first equip the tool, example:

local Player
local Character
local RightHand -- You probably don't need this global variable anymore
-- Once you refactor
local Motor6D

Tool.Equipped:Connect(function()
	local Humanoid = tool:FindFirstAncestorOfClass("Humanoid")
	Character = Humanoid.Parent
	Player = game.Players:GetPlayerFromCharacter(Character)
	RightHand = Character:FindFirstChild("RightHand")
	Motor6D = Instance.new("Motor6D", RightHand)
	Motor6D.Part0 = RightHand
	Motor6D.Part1 = Tool.Sword
end)

Tool.Unequipped:Connect(function()
	Motor6D:Destroy()
	-- Probably set Player, Character, RightHand to nil
end)

The client code looks decent. Using Humanoid’s LoadAnimation is probably fine. I didn’t like (except for play animation event) how you passed strings instead of boolean values to the remote events.

The code looks good overall.

Server Code:

local RunService = game:GetService("RunService")

-- Parent soup is always suspicious to me
local Tool = script.Parent.Parent.Parent.Parent
local Events = Tool.Events

local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock

-- There is no good reason to be lazy here.
-- Use proper variable names
-- seconds, startTime
local function Yield(s)
	local t = tick()
	while tick()-t < s do RunService.Stepped:wait() end
	return tick()-t
end

Tool.Equipped:Connect(function()
	-- Parent soup
	local Character = script.Parent.Parent.Parent.Parent.Parent
	-- You could move this Player variable down to where it's first used
	local Player = game.Players:GetPlayerFromCharacter(Character)
	local RightHand = Character.RightHand
	
	local Motor6D = Instance.new("Motor6D")
	Motor6D.Name = "Motor6D"
	Motor6D.Parent = RightHand
	Motor6D.Part0 = RightHand
	Motor6D.Part1 = Tool.Sword
	
	-- define player here?
	PLAY_ANIMATION_EVENT:FireClient(Player, "Hold")
	CHANGE_MOUSE_EVENT:FireClient(Player, "WeaponMouseIcon")
	SHIFTLOCK_EVENT:FireClient(Player, "Enable")
end)

Tool.Unequipped:Connect(function()
	-- Parent soup
	local Player = script.Parent.Parent.Parent.Parent.Parent.Parent
	-- I've noticed you are using different methods to fetch the
	-- player and character (compared to Equipped), this is suspicous
	local Character = Player.Character
	local RightHand = Character.RightHand
	
	local Motor6D = RightHand.Motor6D
	-- Seems useless to set the properties to nil
	Motor6D.Part0 = nil
	Motor6D.Part1 = nil
	Motor6D:Destroy()
	
	PLAY_ANIMATION_EVENT:FireClient(Player, "StopHold")
	CHANGE_MOUSE_EVENT:FireClient(Player, "DefaultMouseIcon")
	SHIFTLOCK_EVENT:FireClient(Player, "Disable")
end)

Tool.Activated:Connect(function()
	-- Parent soup
	local Player = script.Parent.Parent.Parent.Parent.Parent
	local Values = Tool.Values
	local SwordDebounce = Values.SwingDelay
	
	if SwordDebounce == false then
		SwordDebounce = true
		
		PLAY_ANIMATION_EVENT:FireClient(Player, "Swing")
		
		Yield(.5)
		SwordDebounce = false
	end
end)

Client:

local UIS = game:GetService("UserInputService")

-- Parent soup
local Tool = script.Parent.Parent.Parent.Parent
local Events = Tool:WaitForChild("Events")
local Player = game.Players.LocalPlayer
local Character = Player.Character or Player.CharacterAdded:wait()
local Humanoid = Character.Humanoid
local PlayerScripts = Player.PlayerScripts
local Values = PlayerScripts.Values
local IsHoldingTool = Values.IsHoldingTool
local PlayerGui = Player.PlayerGui
local WeaponMouseIcon = PlayerGui.WeaponMouseIcon
-- Not a fan at all of the CAPITALIZED_SNAKE_CASE
-- Seems out of place
local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock
local AnimationsFolder = Tool.Animations
local SWING_ANIMATION = AnimationsFolder.Swing
local HOLD_ANIMATION = AnimationsFolder.Hold

PLAY_ANIMATION_EVENT.OnClientEvent:Connect(function(AnimationName)
	if AnimationName == "Hold" then
		-- Store/pre load the animation tracks
		-- Each time this event is called, you create a bunch of new animation tracks
		-- LoadAnimation is deprecated, perhaps look into using
		-- the humanoid's Animator instance
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)
		HOLD_ANIMATION_TRACK:Play()
		HOLD_ANIMATION_TRACK:AdjustWeight(1, .1)
	elseif AnimationName == "Swing" then
		local SWING_ANIMATION_TRACK = Humanoid:LoadAnimation(SWING_ANIMATION)
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)
		HOLD_ANIMATION_TRACK:Stop()
		SWING_ANIMATION_TRACK:Play()
	elseif AnimationName == "StopHold" then
		local HOLD_ANIMATION_TRACK = Humanoid:LoadAnimation(HOLD_ANIMATION)
		HOLD_ANIMATION_TRACK:Stop()
	end
end)

-- For these two last events it seems like there are only two states
-- Perhaps you should just use true and false instead of passing a string
-- This is especially true for the shift lock event
CHANGE_MOUSE_EVENT.OnClientEvent:Connect(function(MouseIcon)
	if MouseIcon == "WeaponMouseIcon" then
		UIS.MouseIconEnabled = false
		WeaponMouseIcon.Enabled = true
	elseif MouseIcon == "DefaultMouseIcon" then
		UIS.MouseIconEnabled = true
		WeaponMouseIcon.Enabled = false
	end
end)
-- Rename response to enabled and pass a boolean value,
-- then you can do "if enabled then code"
SHIFTLOCK_EVENT.OnClientEvent:Connect(function(Response)
	if Response == "Enable" then
		IsHoldingTool.Value = true
	elseif Response == "Disable" then
		IsHoldingTool.Value = false
	end
end)

FerbZides · November 27, 2020, 9:22am

hamsterloverboy3:

-- define player here?
	PLAY_ANIMATION_EVENT:FireClient(Player, "Hold")
	CHANGE_MOUSE_EVENT:FireClient(Player, "WeaponMouseIcon")
	SHIFTLOCK_EVENT:FireClient(Player, "Enable")

based on the API of Roblox you need to pass it unlike FireServer() it’s the client that fires it so no need to pass the player argument

hamsterloverboy3:

-- I've noticed you are using different methods to fetch the
	-- player and character (compared to Equipped), this is suspicous
	local Character = Player.Character
	local RightHand = Character.RightHand

any suggestions for this?

hamsterloverboy3:

-- Not a fan at all of the CAPITALIZED_SNAKE_CASE
-- Seems out of place
local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock
local AnimationsFolder = Tool.Animations
local SWING_ANIMATION = AnimationsFolder.Swing
local HOLD_ANIMATION = AnimationsFolder.Hold

any suggestions for this either?

aetherloric · November 27, 2020, 4:32pm

hamsterloverboy3:

-- I've noticed you are using different methods to fetch the
	-- player and character (compared to Equipped), this is suspicous
	local Character = Player.Character
	local RightHand = Character.RightHand

any suggestions for this?

You already defined the Tool, just use

local Character = Tool.Parent;
local Player = game.Players:GetPlayerFromCharacter(Character);

hamsterloverboy3:

-- Not a fan at all of the CAPITALIZED_SNAKE_CASE
-- Seems out of place
local PLAY_ANIMATION_EVENT = Events.PlayAnimation
local CHANGE_MOUSE_EVENT = Events.ChangeMouse
local SHIFTLOCK_EVENT = Events.Shiftlock
local AnimationsFolder = Tool.Animations
local SWING_ANIMATION = AnimationsFolder.Swing
local HOLD_ANIMATION = AnimationsFolder.Hold

any suggestions for this either?
[/quote]
Camel case is the best .

hamsterloverboy3 · November 28, 2020, 6:43am

I wasn’t talking about passing the argument or not. I was saying that you could move your player declaration right next to where it’s used. It’s not used at the top of the block where it’s declared. When I was reading and saw Player, I was wondering where it was used.

You simply can have global variables that are set when a player equips the tool and then you set those variables to nil when the player unequips and do some cleanup.

The main problem with this is not the capitalized snake case, it’s the fact that you have other global variables which do not follow this convention. The mixing of conventions is the problem here. Pick one naming convention for global variables and don’t mix them.

angrybino · February 5, 2021, 7:21am

tick() will soon be deprecated, consider using os.time().

local function Yield(s)
	local t = tick()
	while tick()-t < s do RunService.Stepped:wait() end
	return tick()-t
end

Don’t index services directly via the . operator, instead define them at the top of the script: local service = game:GetService(serviceName) as it is good practice and keeps good consistency in code.

local Player = game.Players:GetPlayerFromCharacter(Character)

When working with debounces, the most efficient way to do is to have a table of cooldowns and whenever the player activates the sword, add that player to the table along with the value being os.time(). Then simply check if the time passed since the last time the player clicked is greater than the cool down which in this case, is .5. This is also efficient since you won’t be yielding unnecessarily and allows accurate debounces.

local COOL_DOWN = .5 -- always have constants for variables that arent expected to change, also prevents magic numbers

local CoolDowns = {}

Tool.Activated:Connect(function()
	-- Parent soup
	local Player = script.Parent.Parent.Parent.Parent.Parent
	
    local coolDown = CoolDowns[Player.Name] -
    --[[ if the player activates the sword for the first time, the player wont be expected to have a cooldown 
         so make sure the coolDown exists
    ]]
    if (coolDown and os.time() - coolDown < COOL_DOWN) then return end

   PLAY_ANIMATION_EVENT:FireClient(Player, "Swing")
   -- update the cooldown
   CoolDowns[Player.Name] = os.time()
end)

I’ve only reviewed your server script, I’ve left the client script to be reviewed by someone else.