Is this bad coding practice?

Help and Feedback Code Review
#1 
function Guard:Shoot(character)
if not character then return end
local player = Players:GetPlayerFromCharacter(character)
if not player then return end

local humanoid = character:FindFirstChildOfClass("Humanoid") 
if not humanoid then return end
local rootpart = humanoid.RootPart 
if not rootpart then return end
end

i use find first child of class to find the player humanoid and to make sure its the actual one rather than an accessory that has the same name.

and i also want to make sure the root part is the humanoid root part.

about the return ends, i put them there so it wouldn’t halt the code.

i cant tell if this is bad coding practice or over skepticism

#2

Personally i’d write it like this:

function Guard:Shoot(character)
   if not character then return end

   local player = Players:GetPlayerFromCharacter(character)
   local humanoid = character:FindFirstChildOfClass("Humanoid") 
   local rootPart = character:FindFirstChild("HumanoidRootPart")

   if not player or not humanoid or not rootPart then return end
   -- code
end
1 Like
#3

Considering it’s in a server script AND the function is also being executed by you, the character being referenced is always 100% the character, you don’t really need all of those checks.

You only do checks when you’re unsure of something, or you are receiving the input from the client and processing it in the sever. You could write something like this:

function Guard:Shoot(character)
    local player = Players:GetPlayerFromCharacter(character)

    local humanoid = character:FindFirstChildOfClass("Humanoid") 
    local rootpart = humanoid:WaitForChild("HumanoidRootPart")
end

It will be much cleaner and easier to debug, since it will throw errors when you pass something wrong, unlike the original code which will return nothing. Also always use :WaitForChild() when referencing something, as you cannot be sure if it has loaded yet.

#4

that sounds good, but some ugc creator might as well name their item “HumanoidRootPart” just to mess with people (im just thinking). and i put return end every line so it wouldn’t do unnecessary calculations.

also i would use character.PrimaryPart, but on r6 the primary part is the head for some reason and the humanoid returns the root part as part of its api

#5

you mean the handle named HumanoidRootPart?
aren’t you checking directly from the character and not every accessory on the character?

1 Like
#6

when you use find first child or wait for child it searches anything with the same name you typed into the code
image
and the accessory name might be the same name you’re looking for and might give an error depending on what you’re doing with it

and for example if i rename every accessory to humanoid or humanoid root part, it might return that instead of the actual one

#7

i believe the accessory’s names inside of the character is the same as in the catalog. you could make some sort of check that checks every character in the game for an accessory named “Humanoid” or “HumanoidRootPart”, and then delete that

#8

true but that might be unnecessary
because root part property is part of humanoid

local humanoid = character:FindFirstChildOfClass("Humanoid") 
local rootpart = humanoid.RootPart
#9

so that basically solves the problem

1 Like