Can this code be anymore cleaner?

Remingling · March 4, 2024, 9:20pm

Hi,

Last time, I was trying to figure out how to use typechecking oop. After learning stuff, I feel that my code is actually very clean and maintainable, and I haven’t felt this in a long time. Is there anyway you can clean this code even more, even though I think it is already clean. Even organizing my methods would be helpful. Thanks!

--!strict

--/ services
local Players = game:GetService("Players")
local RunService = game:GetService("RunService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
--/ directories
local Modules = ReplicatedStorage.Source.Modules
local Classes = ReplicatedStorage.Source.Classes
local Packages = ReplicatedStorage.Packages
--/ requires
local Spring = require(Modules.Spring)
local Trove = require(Packages._Index["sleitnick_trove@1.1.0"]["trove"])
--/ class
local Camera = {}
Camera.__index = Camera

export type ClassType = typeof( setmetatable({} :: {
            CamPart: Part,
            Player: Player,
            CameraAngle: CFrame,
            XOFFSET: number,
            YOFFSET: number,
            ZOFFSET: number,
            Damper: number,
            Speed: number,
            Spring: any,
            Tracking: any,
            Connections: Trove.ClassType,
            Camera: any,
            Character: any
        }, 
        Camera)
)

function Camera.new(Player: Player): ClassType
    local self = {
        Player = Player,
        Tracking = {Player},
        Connections = Trove.new(),
        Spring = Spring.new(Vector3.new()),
        Camera = workspace.CurrentCamera,
        CamPart = Instance.new("Part"),
        Character = nil,
        -- settings
        CameraAngle = CFrame.Angles(math.rad(0), math.rad(0),math.rad(0)),
        XOFFSET = 0,
        YOFFSET = 3,
        ZOFFSET = 10,
        Damper = 100,
        Speed = 10,
    };

    setmetatable(self, Camera)

    self:_init()

    return self
end

--/ private methods
function Camera._characterAdded(self: ClassType, character: Model)
    self.Character = self.Character or character
end

function Camera._setupCharacterAddedFunction(self: ClassType)
    self.Connections:Connect(self.Player.CharacterAdded, function(character: Model) 
            self:_characterAdded(character)
        end)

    if self.Player then
        self:_characterAdded(self.Player.Character:: Model) 
    end
end

function Camera._init(self: ClassType): () -- initilize
    self:_setupCharacterAddedFunction()

    -- properties
    self.CamPart.Anchored = true
    self.CamPart.Name = "CamPart"
    self.CamPart.Parent = workspace
    self.CamPart.CanCollide = false
    self.CamPart.Transparency = 1

    self.Camera.CameraType = Enum.CameraType.Scriptable
    -- set up the damper and speed settings
    self.Spring.Damper = self.Damper 
    self.Spring.Speed = self.Speed
end

function Camera._calculateAveragePosition(self: ClassType)
    local Total = Vector3.zero()

    for _, Track in pairs(self.Tracking) do    
        if Track:IsA("Player") then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart")
            Total += (Root.Position - self.CamPart.Position).Magnitude
        end

        if Track:IsA("Part") then
            Total += Track.Position
        end
    end

    return Total / #self.Tracking
end

function Camera._calculateAverageMagnitude(self: ClassType)
    local Total = 0

    for _, Track in pairs(self.Tracking) do    
        if Track:IsA("Player") then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart")
            Total += (Root.Position - self.CamPart.Position).Magnitude
        end

        if Track:IsA("Part") then
            Total += (Track.Position - self.CamPart.Position).Magnitude
        end
    end

    return Total / #self.Tracking
end

--/ public 

--/ removes player from self.Tracking -> stops tracking the player
function Camera.RemovePlayerFromTracking(self: ClassType, player: Player): ()
    local index = table.find(self.Tracking, player)

    if index then
        table.remove(self.Tracking, index)
    end
end

--/ adds player -> self.Tracking -> tracks the player
function Camera.AddPlayerToTracking(self: ClassType, player: Player): ()
    if table.find(self.Tracking, player) then
        warn(player.Name.." is already added")
    else
        table.insert(self.Tracking, player)
    end
end

--/ gets the current tracking table
function Camera.GetTrackingTable(self: ClassType): ()
    return self.Tracking
end

--/ update the camera on BindToRenderStep
function Camera.Update(self: ClassType): ()	
    while self.Tracking ~= 0 do
        warn("No player is seen to track.")
        task.wait(1)
    end
    
    local Root = self.Character:FindFirstChild("HumanoidRootPart")
    local AveragePos = self:_calculateAveragePosition()
    local AverageMagnitude = self:_calculateAverageMagnitude() + self.ZOFFSET or self.ZOFFSET

    local StartCF = 
    CFrame.new((Root.CFrame.Position)) 
    * CFrame.Angles(0,math.rad(self.XOFFSET),0)
    * CFrame.Angles(math.rad(self.YOFFSET),0,0)

    local GoalCF = 
    StartCF:ToWorldSpace(
        CFrame.new(self.XOFFSET,self.YOFFSET , AverageMagnitude))

    local CameraDirection = StartCF:ToWorldSpace(
        CFrame.new(self.XOFFSET, self.YOFFSET, -10000)) 
    * CFrame.new(self.CamPart.Position)

    local CameraCF = CFrame.new(self.Spring.Position, CameraDirection.Position) 
        * self.CameraAngle

    self.Spring.Target = GoalCF.Position
    self.CamPart.Position = AveragePos
    self.Camera.CFrame = CameraCF
end

--/ enables or disables the camera
function Camera.Enable(self: ClassType, enable: Boolean):
    if enable then -- on enabled
        local function _update()
            self.Connections:BindToRenderStep("Camera",
                 Enum.RenderPriority.Camera.Value + 1, _update)
        end
        
        self:Update()
    end
    --/ seperated instead of an else statement to make it readable
    if not enable then -- on not enabled
        self.Disconnections:Clean()
    end
end

return Camera

PS: any advice that I think is useful will be updated inside the code

this camera is also for a street fighter inspo game

GFink · March 4, 2024, 9:27pm

Just taking a cursory look, it’s pretty good. I would advise against calling the type self by that name, though. Maybe call it Camera, or CameraInstance if you wanna have no naming conflicts. You wanna make type and variable names clear as to what data they contain.

Remingling · March 4, 2024, 9:41pm

Now that I look at it, I agree with that. So what I think is the best way to fix this, is to do this:

-- removing the "type self"
export type ClassType = typeof( setmetatable({} :: {
	CamPart: Part,
	Player: Player,
	CameraAngle: CFrame,
	XOFFSET: number,
	YOFFSET: number,
	ZOFFSET: number,
	Damper: number,
	Speed: number,
	Spring: any,
	Tracking: any,
	Connections: Trove.ClassType,
	Camera: any,
	Character: any
}, Camera) )

kingerman88 · March 5, 2024, 6:36pm

That is some pretty clean code

I will throw a couple of recommendations.

anklebandit49:

function Camera.RemovePlayerFromTracking(self: ClassType, player: Player): ()
    if table.find(self.Tracking, player) then
        for i = 1, #self.Tracking do
            if self.Tracking[i] == player then
                table.remove(self.Tracking, i)
            end
        end
    end
end

Since table.find returns the index and I there should only be one occurrence of the player, you should be able to do this instead:

function Camera.RemovePlayerFromTracking(self: ClassType, player: Player): ()
    local index = table.find(self.Tracking, player)
    if index then
        table.remove(self.Tracking, index)
    end
end

If there’s multiple players, a while / repeat loop should also work and look decent.

You may also want to consider using the generalized iteration for for loops. Syntax - Luau

That being:

local someTable = {1, 2, 3, 4}
for i,v in someTable do
    -- ...
end

Just a convenience thing so you don’t have to worry about choosing pairs / ipairs at each situation.

In terms of variable naming, I’d recommend renaming the first Root variable to something else as later on Root clearly means HumanoidRootPart rather than a position. Just for consistency and code copying later down the line.

anklebandit49:

        if Track:IsA("Player") and Track.Character then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart").Position
            Total += Root
        end

^ Root is a position
v Root is a Part

anklebandit49:

        if Track:IsA("Player") then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart")
            Total += (Root.Position - self.CamPart.Position).Magnitude
        end

Final nitpicks, some of these you can probably skip on if you want.

Binary operator to remove the if statement into one line. Up to you whether or not you want to implement based on how readable you think this formating is.

    -- becomes
    self.Character = self.Character or character

While loop instead of if statement and repeat loop. Similar to above, that choose the code you think is most readable and you will understand down the line.

    while self.Tracking ~= 0 do
        warn("No player is seen to track.")
        task.wait(1)
    end

Probably wouldn’t abbreviate HumanoidRootPart as Root, I’d just write it out in full just out of habit since it’s just a tad more clear and makes copying / pasting the code easier.
Vertor3.zero as a replacement for the empty Vertor3.new() call. It’s slightly more readable and also a bit more performant (but that’d be negligible unless you’re doing tons of vector math). Still not a bad habit to get into using the Vector3 globals
Move the _setupCharacterAddedFunction under the _init function and _characterAdded under that. In terms of execution, since _init uses _setupCharacterAddedFunction which uses _characterAdded it makes sense for you to leave those methods closer together.

Remingling · March 5, 2024, 8:59pm

Thanks for the indepth review. I like all of these suggestions as they make the code readable . I also changed the code to make it more readable too.

I removed the Disable method and put a boolean parameter inside of the Enable method.
comments are more clear and understandable
ur suggestions are now in the new code

This might be the cleanest it can get:

i’ll solution you if no one replies

new code (will be in the top too):

--!strict

--/ services
local Players = game:GetService("Players")
local RunService = game:GetService("RunService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")
--/ directories
local Modules = ReplicatedStorage.Source.Modules
local Classes = ReplicatedStorage.Source.Classes
local Packages = ReplicatedStorage.Packages
--/ requires
local Spring = require(Modules.Spring)
local Trove = require(Packages._Index["sleitnick_trove@1.1.0"]["trove"])
--/ class
local Camera = {}
Camera.__index = Camera

export type ClassType = typeof( setmetatable({} :: {
            CamPart: Part,
            Player: Player,
            CameraAngle: CFrame,
            XOFFSET: number,
            YOFFSET: number,
            ZOFFSET: number,
            Damper: number,
            Speed: number,
            Spring: any,
            Tracking: any,
            Connections: Trove.ClassType,
            Camera: any,
            Character: any
        }, 
        Camera)
)

function Camera.new(Player: Player): ClassType
    local self = {
        Player = Player,
        Tracking = {Player},
        Connections = Trove.new(),
        Spring = Spring.new(Vector3.new()),
        Camera = workspace.CurrentCamera,
        CamPart = Instance.new("Part"),
        Character = nil,
        -- settings
        CameraAngle = CFrame.Angles(math.rad(0), math.rad(0),math.rad(0)),
        XOFFSET = 0,
        YOFFSET = 3,
        ZOFFSET = 10,
        Damper = 100,
        Speed = 10,
    };

    setmetatable(self, Camera)

    self:_init()

    return self
end

--/ private methods
function Camera._characterAdded(self: ClassType, character: Model)
    self.Character = self.Character or character
end

function Camera._setupCharacterAddedFunction(self: ClassType)
    self.Connections:Connect(self.Player.CharacterAdded, function(character: Model) 
            self:_characterAdded(character)
        end)

    if self.Player then
        self:_characterAdded(self.Player.Character:: Model) 
    end
end

function Camera._init(self: ClassType): () -- initilize
    self:_setupCharacterAddedFunction()

    -- properties
    self.CamPart.Anchored = true
    self.CamPart.Name = "CamPart"
    self.CamPart.Parent = workspace
    self.CamPart.CanCollide = false
    self.CamPart.Transparency = 1

    self.Camera.CameraType = Enum.CameraType.Scriptable
    -- set up the damper and speed settings
    self.Spring.Damper = self.Damper 
    self.Spring.Speed = self.Speed
end

function Camera._calculateAveragePosition(self: ClassType)
    local Total = Vector3.zero()

    for _, Track in pairs(self.Tracking) do    
        if Track:IsA("Player") then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart")
            Total += (Root.Position - self.CamPart.Position).Magnitude
        end

        if Track:IsA("Part") then
            Total += Track.Position
        end
    end

    return Total / #self.Tracking
end

function Camera._calculateAverageMagnitude(self: ClassType)
    local Total = 0

    for _, Track in pairs(self.Tracking) do    
        if Track:IsA("Player") then
            local Root = Track.Character:FindFirstChild("HumanoidRootPart")
            Total += (Root.Position - self.CamPart.Position).Magnitude
        end

        if Track:IsA("Part") then
            Total += (Track.Position - self.CamPart.Position).Magnitude
        end
    end

    return Total / #self.Tracking
end

--/ public 

--/ removes player from self.Tracking -> stops tracking the player
function Camera.RemovePlayerFromTracking(self: ClassType, player: Player): ()
    local index = table.find(self.Tracking, player)

    if index then
        table.remove(self.Tracking, index)
    end
end

--/ adds player -> self.Tracking -> tracks the player
function Camera.AddPlayerToTracking(self: ClassType, player: Player): ()
    if table.find(self.Tracking, player) then
        warn(player.Name.." is already added")
    else
        table.insert(self.Tracking, player)
    end
end

--/ gets the current tracking table
function Camera.GetTrackingTable(self: ClassType): ()
    return self.Tracking
end

--/ update the camera on BindToRenderStep
function Camera.Update(self: ClassType): ()	
    while self.Tracking ~= 0 do
        warn("No player is seen to track.")
        task.wait(1)
    end
    
    local Root = self.Character:FindFirstChild("HumanoidRootPart")
    local AveragePos = self:_calculateAveragePosition()
    local AverageMagnitude = self:_calculateAverageMagnitude() + self.ZOFFSET or self.ZOFFSET

    local StartCF = 
    CFrame.new((Root.CFrame.Position)) 
    * CFrame.Angles(0,math.rad(self.XOFFSET),0)
    * CFrame.Angles(math.rad(self.YOFFSET),0,0)

    local GoalCF = 
    StartCF:ToWorldSpace(
        CFrame.new(self.XOFFSET,self.YOFFSET , AverageMagnitude))

    local CameraDirection = StartCF:ToWorldSpace(
        CFrame.new(self.XOFFSET, self.YOFFSET, -10000)) 
    * CFrame.new(self.CamPart.Position)

    local CameraCF = CFrame.new(self.Spring.Position, CameraDirection.Position) 
        * self.CameraAngle

    self.Spring.Target = GoalCF.Position
    self.CamPart.Position = AveragePos
    self.Camera.CFrame = CameraCF
end

--/ enables or disables the camera
function Camera.Enable(self: ClassType, enable: Boolean):
    if enable then -- on enabled
        local function _update()
            self.Connections:BindToRenderStep("Camera",
                 Enum.RenderPriority.Camera.Value + 1, _update)
        end
        
        self:Update()
    end
    --/ seperated instead of an else statement to make it readable
    if not enable then -- on not enabled
        self.Disconnections:Clean()
    end
end

return Camera

system · March 19, 2024, 9:00pm

This topic was automatically closed 14 days after the last reply. New replies are no longer allowed.