Thoughts on "Player" constructor // am I using knit right?

Help and Feedback Code Review
#1

just getting started with knit! i definitely see how useful it could be, and ive been attempting to use it for my upcoming game. so far, everything works well and i haven’t had many direct issues, but i wanted to double check that I am using knit correctly and not being wasteful of my time and/or performance!

also was curious if the idea of creating a new object for each player is worth it (especially the characteradded system)

worth noting this is just the foundation and code isn’t finished or anything

--@author:SpiralAPI

--//Services
local Players = game:GetService("Players")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

--//Imports
local Knit = require(ReplicatedStorage.Packages.Knit)

local Trove = require(ReplicatedStorage.Packages.Trove)
local Observers = require(ReplicatedStorage.Packages.Observers)
local Timer = require(ReplicatedStorage.Packages.Timer)
local TableUtil = require(ReplicatedStorage.Packages.TableUtil)
local Signal = require(ReplicatedStorage.Packages.Signal)

local ProfileService = require(ReplicatedStorage.Packages.ProfileService)
local ProfileTemplate = require(ReplicatedStorage.Data.Profile)


--//KnitServices
local PlayerService = Knit.CreateService({
	Name = "PlayerService",
	Players = {};
	SetAddedTime = Signal.new();
	Client = {
		
	};
})


--//Variables
local Key = "DataV3-DEV0.0.1"
local ProfileStore = ProfileService.GetProfileStore(
	Key,
	ProfileTemplate.Default
)

local Tag = ReplicatedStorage.Assets.Visuals.Tag

--//Private Functions
function ProfileAdded(Player, Profile)	
	--creates leaderstats/attributes (uses observers module)
	end
end

function LoadProfile(Player)
	--basic profileservice function
end

function NewPlayer(RealPlayer: Player)
	--Constructor
	local Player = {}; Player.__index = Player
	Player._trove = Trove.new()
	
	--Variables
	Player.Player = RealPlayer
	Player.Profile = LoadProfile(RealPlayer)
	Player.CharacterAdded = {
		function(Character)
			local HumanoidRootPart = Character:FindFirstChild("HumanoidRootPart")

			local NewTag = Tag:Clone();
			NewTag.Parent = HumanoidRootPart
			NewTag.Username.Text = RealPlayer.Name
		end; --TODO: Add tags/cosmetics more efficiently
	}
	Player.JoinedAt = workspace:GetServerTimeNow()
	Player.AddedTime = 0

	--Private Functions
	local function CharacterAdded(Character)
		for _, Function in pairs(Player.CharacterAdded) do
			task.spawn(Function, Character)
		end
	end

	--Public Functions
	function Player:GetData()
		if Player.Profile then
			return Player.Profile.Data
		end
	end

	function Player:Destroy()
		if Player.Profile then
			Player.Profile:Release()
		end
		Player._trove:Destroy()
	end

	function Player:SetAddedTime(Operation: "Add" | "Subtract" | "Set", Amount)
		Player.AddedTime = (Operation == "Add" and Player.AddedTime + Amount) or (Operation == "Subtract" and Player.AddedTime - Amount) or (Operation == "Set" and Amount)
	end

	--Init
	task.spawn(CharacterAdded, RealPlayer.Character or RealPlayer.CharacterAdded:Wait())
	Player._trove:Add(RealPlayer.CharacterAdded, CharacterAdded)

	return Player
end

--//Public Functions
function PlayerService:ChangeLeaderstat(Player: Player, Type: "Time" | "Top" | "Kills", Operation: "Add" | "Subtract" | "Set", Amount: number)
	if (self.Players[Player] and Player:FindFirstChild("leaderstats")) then
		local Stat = Player:FindFirstChild("leaderstats")[Type]
		if Stat then
			Stat.Value = (Operation == "Add" and Stat.Value + Amount) or (Operation == "Subtract" and Stat.Value - Amount) or (Operation == "Set" and Amount)
		end
	end
end

function PlayerService:GetData(Player)
	return self.Players[Player] and self.Players[Player]:GetData()
end

function PlayerService.Client:GetData(Player)
	return self.Server:GetData(Player)
end

function PlayerService:KnitStart()
	--FUNCTIONS
	local function MakePlayer(Player)
		local PlayerObj = NewPlayer(Player)
		self.Players[Player] = PlayerObj
	end

	--CONNECTIONS
	for _, Player in pairs(Players:GetPlayers()) do
		task.spawn(MakePlayer, Player)
	end
	Players.PlayerAdded:Connect(MakePlayer)

	Players.PlayerRemoving:Connect(function(Player)
		self.Players[Player]:Destroy()
		self.Players[Player] = nil
	end)

	--MAIN TIME LOOP
	local CoreTimeLoop = Timer.new(1)
	CoreTimeLoop.Tick:Connect(function()
		for _, Player in pairs(Players:GetPlayers()) do
			if (not shared:IsSafe(Player)) and self.Players[Player] then
				self:ChangeLeaderstat(Player, "Time", "Add", 1 + self.Players[Player].AddedTime)
			end
		end
	end)
	CoreTimeLoop:Start()
end

--//KnitService
return PlayerService
4 Likes
#3

image

2 Likes
#5

then dont reply :person_shrugging: attention seeking on the devforum is astronomical

3 Likes
#6

PSA: Don’t use Knit because

1 Like
#7

honestly really interesting read! definitely changed my perspective on things. however, knit aside, can i get input on the code itself as well? mainly the constructor for the player

#8

  1. I reckon handling CharacterAdded by yourself is unnecessary. Leave that to Roblox. It’s not a developer’s responsibility and, plus, doing it the normal way doesn’t hurt.

  2. Maybe use shared.

  3. Use OOP instead of a singleton (part of the Knit migration)

  4. Just use if statements. Or, even better, create a respective method for each and all cases, instead of handling them all in a single function.

function PlayerService:ChangeLeaderstat(Player: Player, Type: "Time" | "Top" | "Kills", Operation: "Add" | "Subtract" | "Set", Amount: number)
    ...
    Stat.Value = (Operation == "Add" and Stat.Value + Amount) or (Operation == "Subtract" and Stat.Value - Amount) or (Operation == "Set" and Amount)
    ...
end
  1. Nitpicks.
    1. You need only use FindFirstChild once after you guarantee the instance’s existence.
    1. ipairs for arrays, pairs for dictionaries.
    1. Luau has introduced if expressions some time ago. Use them like so: if condition then expr else expr. This is meant to replace these cases and and or, since they can get pretty ugly pretty quickly.