Better Way to do this?

Help and Feedback Code Review
, ,
#1

Hey, so I have this script:

local UIS = game:GetService('UserInputService')
local Players = game:GetService("Players")
local Player = Players.LocalPlayer
local Character = Player.Character or Player.CharacterAdded:Wait()


UIS.InputBegan:connect(function(input,processed)
	if processed then return end
 	if input.KeyCode == Enum.KeyCode.LeftShift then
  		Character.Humanoid.WalkSpeed = 24
 		local Anim = Instance.new('Animation')
		Anim.AnimationId = 'rbxassetid://10910827063'
 		PlayAnim = Character.Humanoid:LoadAnimation(Anim)
  		PlayAnim:Play()
 	end
end)


UIS.InputEnded:connect(function(input)
 	if input.KeyCode == Enum.KeyCode.LeftShift then
		Character.Humanoid.WalkSpeed = 16
		if PlayAnim then
			PlayAnim:Stop()
			PlayAnim:Destroy()
		end
 	end
end)

What it does:
Simply giving the player running animation when running [ pressing shift].

My Question:
How could I improve this? Although it works, is there a better way to do this?

1 Like
#2

Load animation once and just :Play() and :Stop() it instead of loading it each time you press the shift.

#3

I mean, I need to load it in order to play it.

#4

Yes you have to, but what is the problem to just load it once or everytime character spawns?

#5

I mean , yeah it sounds better, but that didnt work.

#6

Adding onto this reply, everytime the shift button is
pressed, you instance a new Animation instance.

Imagine someone sprinting 50 times, that’s 50 unneeded Animation instances being created.

To best solve this, either create it manually in studio and put it someone where it can be easily referenced, which is what I would recommend, or only define it once outside of your connection.

Also, make sure to call :Destroy() if needed when using the latter, depending on what type of script this is or where you decide to parent it.

#7

What do you mean, I’ve defined it already.

#8

What I mean is instancing your Animation instance outside of the InputBegan connection, so you only create it once, and not everytime the shift button is pressed.

#9

Mhm. But doesnt the “destory” delete it when the player stops holding it?

#10

Yes, but in the name of improving the readability and the very minor micro-optimizations, instancing it everytime is a bit of a bad practice unless truly needed.

#11 
local UIS = game:GetService('UserInputService')
local Players = game:GetService("Players")
local Player = Players.LocalPlayer
local Character = Player.Character or Player.CharacterAdded:Wait()
local Humanoid = Character:WaitForChild("Humanoid")
local Anim = Instance.new('Animation')
Anim.AnimationId = 'rbxassetid://10910827063'

UIS.InputBegan:connect(function(input,processed)
	if processed then return end
 	if input.KeyCode == Enum.KeyCode.LeftShift then
		Humanoid.WalkSpeed = 24
		PlayAnim = Humanoid:LoadAnimation(Anim)
  		PlayAnim:Play()
 	end
end)

Humanoid:GetPropertyChangedSignal("MoveDirection"):Connect(function()
	if Humanoid.MoveDirection.Magnitude == 0 then
		if PlayAnim then
			PlayAnim:Stop()
			PlayAnim:Destroy()
		end
	end
end)

UIS.InputEnded:connect(function(input)
 	if input.KeyCode == Enum.KeyCode.LeftShift then
		Humanoid.WalkSpeed = 16
		if PlayAnim then
			PlayAnim:Stop()
			PlayAnim:Destroy()
		end
 	end
end)

Better?