Feedback on my spectate function

Help and Feedback Code Review
#1 
local index = 1 

local function Spectate(bool)
	local PlayersAvailableToSpecate = Functions.GetPlayersAvailableForSpectate:InvokeServer()
	
    if CurrentCamera.CameraType ~= Enum.CameraTyupe.Custom then
	    CurrentCamera.CameraType = Enum.CameraType.Custom
    end
	
	if bool then
		if index >= 1 and < #PlayersAvailableToSpectate then
			index += 1
		end
	else
		if index > 1 then
			index -= 1
		end
	end
	
	local character = PlayersAvailableToSpecate[index].Character or PlayersAvailableToSpecate[index].CharacterAdded:Wait()
        
        if CurrentCamera.CameraSubject ~= character then
	     CurrentCamera.CameraSubject = character
     end
end

Here’s what I want reviewed:

  • Code Readability
  • Efficiency

Note: I send a remote function to the server to get the current players in round and then the server returns the amount of players back to the client.

1 Like
#2

I think your code is ok in terms of readability, however, it doesn’t work how you want it to.

In my opinion, the INDEX variable is unnecessarily capitalized and it is not consistent with the rest of the codes variable naming scheme, it should be Index to be consistent with the rest of the code using PascalCase for variable naming.

It is also specific to the scope, so whenever you call Spectate(), INDEX will always either be 0, or 2.
To fix that you simply put the variable outside of the scope of Spectate(), say right above the Spectate() function declaration would work fine.

You should probably name the bool parameter to be something a bit more descriptive, as it did not occur for me what it was for at the first. Maybe something like CycleAdd?

There’s another issue that will occur once you put the INDEX variable outside of the scope, which is that there is no respect for the number of players there are.
Say there are 5 players, there’s no respect to that, so a user could cycle to 10, and who would they be spectating at that point?

To solve that, you can incorporate some modulus when you’re adding to INDEX and subtracting to INDEX.
For addition, you can do INDEX = (INDEX % #PlayersAvailableToSpectate) + 1,
and for subtraction you can do INDEX = ((INDEX - 2) % 10) + 1

I don’t see why you’re using an pcall when setting the camera subject.
You can just check if the indexed player exists, which if it does not you don’t go further with your camera subject setting code.

You also wait for the character to be added incorrectly, you’re meant to call :Wait() on an event, such as Player.CharacterAdded, not on a model which it will error when you try to use :Wait() on it.

You probably also meant to set the camera subject to the HumanoidRootPart, or the Humanoid, as setting the camera subject to be the model might not give you the expected behavior.

The camera setting code should look something like this:

local IndexedPlayer = PlayersAvailableToSpectate[INDEX]
if not IndexedPlayer then
	return
end
local Character = IndexedPlayer.Character or IndexedPlayer.CharacterAdded:Wait()
-- Waiting for the HumanoidRootPart to be added, 
-- as sometimes when the CharacterAdded() event fires, the HumanoidRootPart is not already there.
local HumanoidRootPart = Character:WaitForChild("HumanoidRootPart")

CurrentCamera.CameraSubject = HumanoidRootPart

In terms of efficiency, there isn’t a lot of content in the script so there isn’t a lot to optimize, it seems to be as efficient as it can be already except for the pcall.

There’s also a minor typo in the PlayersAvailableToSpecate, Spectate is written wrongly.

1 Like
#3
#4