Client Utilities

Help and Feedback Code Review
#1

I wrote some code to avoid repeating code/duplicating objects across multiple other clientside scripts that require animations, humanoids, etc. However, I am not sure if this is a good practice. Should I be doing this at all, or should I just access the humanoid from every tool’s script manually, and access the animator manually, and load the animations manually, for every single one? Alternatively, is this a good use case for _G?

local Module = {}
do
	local Slash = Instance.new("StringValue")
	Slash.Name, Slash.Value, Module.Slash = "toolanim", "Slash", Slash
end
local Animation = Instance.new("Animation")
game:GetService("Players").LocalPlayer.CharacterAdded:Connect(function(Char)
	local Table, Humanoid:Humanoid = {}, Char:WaitForChild("Humanoid")
	local Animator:Animator = Humanoid:WaitForChild("Animator")
	Module.Character, Module.Humanoid, Module.HipHeight, Module.Animation = Char, Humanoid, Vector3.new(nil, Humanoid.HipHeight), function(Id)
		local Anim:AnimationTrack? = Table[Id]
		if not Anim then
			Animation.AnimationId = Id
			Anim, Animation.AnimationId = Animator:LoadAnimation(Animation)
			Table[Id] = Anim
		end
		return Anim
	end
end)
return Module
2 Likes
#2

It’s generally a best to avoid code duplication and create reusable code, so what you’ve done is a smart and a good practice.

Here are some aspects of the code that can be improved:

  1. Variable naming: Some of the variable named in this code are not very descriptive. Such as “Table” and “Anim”. It’s generally best to use more descriptive names that convey the purpose of the variable.

  2. Error handling: Your code does not include any error handling, which can make it difficult to debug if something goes wrong. It would be better to include appropriate error handling, such as pcall functions or even just print statements.

  3. Code organization: This code is relatively short, it could still benefit from better organization. For example, it would be helpful to group related functionality into separate functions, rather than having everything in one block of code. It creates better scalability if there are other tools you want animate. As well as easier readability for those who don’t immediately know what the script is pertaining.

2 Likes
#3

Though if its only you reading this code, it doesnt matter too too much. Though again I do recommend error statments.

#4

I’m not particularly sure how to feel about assigning multiple values in a single line, especially the 2nd case. Module.Animation should probably get its own line, or you could use some clearer syntax like:

function Module.Animation(id)
    -- blah blah blah
end

There is almost never a good use case for _G.

  1. It’s hard to debug. If you make another script that changes, for example _G.Character, it might break something. It would be very hard to find where that change came from, unlike modules.
  2. You would need to yield, or wait, to be sure that it had actually loaded all the values in _G. With modules, you don’t need to do that.
1 Like