Does my code look good?

basically i’ve made a module script that which gets the closest player from any given position as long as its within range

local Players = game:GetService("Players")

local function findClosestPlayer(position, range)
	local closestPlayer
	local closestDistance = range
	for _, player in pairs(Players:GetChildren()) do
		local distance = player:DistanceFromCharacter(position)
		if distance <= closestDistance then
			closestDistance = distance
			closestPlayer = player
		end
	end
	if not closestPlayer then return nil end
	if not closestPlayer.Character then return nil end
	return closestPlayer
end

return findClosestPlayer

Looks good! There’s a slight issue though and a few lines that are redundant.

The function player:DistanceFromCharacter(position) returns 0 if there is no player.Character which will break your function. You should wrap the handler of your for loop inside an if statement checking if the player has a character.

Like so:

local Players = game:GetService("Players")

local function findClosestPlayer(position, range)
	local closestPlayer
	local closestDistance = range
	for _, player in pairs(Players:GetChildren()) do
		if player.Character then -- Checks if player has 
			local distance = player:DistanceFromCharacter(position)
			if distance <= closestDistance then
				closestDistance = distance
				closestPlayer = player
			end
		end
	end
	if not closestPlayer then return nil end
	if not closestPlayer.Character then return nil end
	return closestPlayer
end

return findClosestPlayer

Now that we got that down, there are a few lines that are redundant. At the bottom of the function you are returning nil if closestPlayer is nil. It would be better to just return closestPlayer because if there is no closestPlayer than nil will be returned anyways.

The line under that is redudant aswell because with the change I added, the closestPlayer will always have a character.

Leaving you with this:

local Players = game:GetService("Players")

local function findClosestPlayer(position, range)
	local closestPlayer
	local closestDistance = range
	for _, player in pairs(Players:GetChildren()) do
		if player.Character then -- Checks if player has 
			local distance = player:DistanceFromCharacter(position)
			if distance <= closestDistance then
				closestDistance = distance
				closestPlayer = player
			end
		end
	end
	return closestPlayer
end

return findClosestPlayer
1 Like