I had an OOP system that creates something for a player (OOP probably isn’t necessary for this but yeah) and I save it in a dictionary of the same thing, but for other players and then when the clients need to get it I clone its table and send it to the client it sounds very confusing I know so here’s the code for it.
function BeyManager.new(character,beyName)
warn("[SERVER]: creating a new bey")
local beyName = beyName or Data:GetData(Players:GetPlayerFromCharacter(character),"currentBey")
if not beys:FindFirstChild(beyName) then warn("[SERVER]: " .. beyName .." does not exist") return end
local beyModel = beys:FindFirstChild(beyName):Clone()
beyModel.Name = character.Name.."s Bey"
beyModel.Parent = RepStorage.CreatedBeys
local self = {}
self.model = beyModel
self.name = self.model.Name
self.root = self.model.Root
self.ownersCharacter = character
self.baseStamina = 15 + math.random(20,50)
self.baseDamage = 2 + math.random(20,50)
self.ownerIsPlayer = Players:GetPlayerFromCharacter(character) or false
self.config = self.model.Configuration
beyInstances[character.Name.."s Bey"] = self
warn("Successfully created a new bey",beyInstances)
return setmetatable(self,BeyManager)
end
local function LaunchBey(character)
warn("[SERVER]: Launching bey")
if not BeyManager:GetBey(character) then warn("[SERVER]: Couldn't get bey") return end
if activeBeys[BeyManager:GetBey(character).name] then warn("[SERVER]: bey is already active") return end
local bey = BeyManager:GetBey(character)
local beyClone = Utils:DeepCopy(bey) --basically a clone of beyInstances but accessible by the client
local clone = beyClone.model:Clone()
beyClone.model = clone
beyClone.root = clone.Root
beyClone.name = clone.Name
beyClone.connections = {}
beyClone.root.CFrame = character.HumanoidRootPart.CFrame + Vector3.new(0,0,3)
beyClone.model.Parent = workspace.PlayerBeys
MakeHitboxPart(beyClone)
beyClone.root:SetNetworkOwner(Players:GetPlayerFromCharacter(character))
Effects:FireAllClients("Launcher","Equip",character)
activeBeys[character.Name.."s Bey"] = beyClone
for _,value in pairs(bey.config:GetChildren()) do
beyClone.connections[value.Name.."Listener"] = value:GetPropertyChangedSignal("Value"):Connect(function()
warn("VALUE CHANGED")
beyClone.model.Configuration:FindFirstChild(value.Name).Value = value.Value
end)
end
beyClone.connections["healthListener"] = beyClone.model.Humanoid:GetPropertyChangedSignal("Health"):Connect(function()
if beyClone.model.Humanoid.Health <= 0 then
ReturnBey(character)
end
end)
return activeBeys[character.Name.."s Bey"]
end
--returns the given characters bey and specific property to it (if it's provided)
-- (used to be deep cloned and put in activeBeys)
function BeyManager:GetBey(character,specificProperty)
warn("[SERVER]: getting bey")
if not beyInstances[character.Name.."s Bey"] then return end
if specificProperty then return beyInstances[character.Name.."s Bey"][specificProperty] end
return beyInstances[character.Name.."s Bey"]
end
Wanted to know how I could do this better or if it’s fine the way it is.
Posted the full code here full code
This is just a personal preference, but, I dislike using self as a variable name, as it can be confusing sometimes if it’s used in describing the array itself or just a variable.
value.Changed is not the same as value:GetPropertyChangedSignal("Value"). Since we have the ability to strictly specify what properties we want to listen for, it’s probably best to isolate our cases to just changes in Value so we aren’t updating if other properties of the ValueBase are altered.
I found this code slightly difficult to read because you use the term bey instead of the idiomatic/conventional self to reference this current object. I would suggest switching it to self just so other readers find it easier to skim and figure out what you’re trying to do.
I feel like the OOP use here is close to being justifiable but just barely falls short; there are methods here that make use of the state of the current BeyManager object, but there aren’t enough to make it worth using classes/the __index metamethod.
A lot of the methods in the class are methods that are included in the class but do nothing to read or write to the fields in the current object. Take this method for example:
function BeyManager:UpdateBey(character,value)
local bey = BeyManager:GetBey(character)
bey.config.Speed.Value = value
end
This is hard to justify because:
The inclusion of local bey = BeyManager:GetBey(character) makes this close to a pure function (a function where its output is identical for a specific input). It seems like you’re making an alias for a statically-called function, which is kind of a roundabout way of doing things.
This shows that the BeyManager class itself is being used as a table of references and doesn’t hold much to determine state. A regular table would suit this use case perfectly fine.
Until you have methods that make better use of and/or alter the fields of the object, I think you should revert to a more direct implementation that makes use of regular tables.
TL;DR: I think this class needs to make better use of self/bey to make it as “efficient” as you desire. Using OOP in this manner (and perhaps carrying this kind of design structure to other classes) will start to convolute your workflow and make using the code harder to read and use in the future.
Is there any reason that LaunchBey isn’t wrote into your class? You would be more or less using the same amount of memory with the code differences that result from that change. The whole point of OOP is inheritance, if you aren’t using self consistently then you really shouldn’t be using OOP at all.
A good rule of thumb is that if your class is anything less than a constructor function & at least two methods, you probably don’t need to write it as a class. This would be met by reorganizing the module’s private functions into the class itself instead.
This, and it’s far less idiomatic and ideal to write a whole bunch of new variables to the stack when you could just let a metatable do its actual job as far as classes are concerned. Lua.org explains it decently, but if you read here, python explains it better. The main difference being that Lua structures are always led back to tables, so self will instead represent your previous argument which is generally table type.
One thing to keep in mind is that everyone will organize their game differently, a metatable has the exact same cost in memory as a normal table, because that’s all that it is. You could write this either way and be fine. It’s wasteful though, to keep creating variables in these private functions that would better serve their purpose under the BeyManager table instead.
One thing I’d like to mention, in regards to the conversation above, is that metatables have more versatility than just __index with regard to constructors. A lot of these custom listeners can meet their purpose just as well if not better using __newindex & __index + a proxy table to act as a .Changed event for all of your object’s information (IE, everything you write into self within your constructor).
It’s actually a pretty common practice. It helps make the constructor visually more consistent with other programming languages like Java.
