I’ve decided to learn OOP to organize my code just today, but I’m unsure how to approach some of these scenarios. I realized that I don’t know to organize it when I was working on my game. I just watched one video on OOP so I’m sure there’s a lot of things wrong with my code, any corrections would be great.
My goal is to make it so that when a part is touched, both parts take damage.
My approach to this is here:
Class script:
local BlockClass = {}
BlockClass.__index = BlockClass
function BlockClass.new(block, cframe, parent)
if cframe == nil then return end
if block == nil then return end
local blockData = {}
setmetatable(blockData, BlockClass)
local blockModel
for _, uniBlockList in pairs(game.ReplicatedStorage.Blocks:GetChildren()) do
if uniBlockList:FindFirstChild(block) then
blockModel = uniBlockList:FindFirstChild(block)
break
end
end
if not blockModel then return end
if not blockModel:FindFirstChild("BlockInfo") then return end
blockData.Model = blockModel:Clone()
blockData.Info = require(blockModel.BlockInfo)
blockData.Health = blockData.Info.Health
blockData.Model.Parent = parent or game.Workspace
blockData.Model.PrimaryPart = blockData.Info.PrimaryPart
blockData.Model:SetPrimaryPartCFrame(cframe)
if blockData.Info.onStart then
spawn(function()
blockData.Info.onStart()
end)
end
return blockData
end
function BlockClass:TakeDamage(obstacle)
if obstacle and obstacle:FindFirstChild("ObstacleInfo") then
local obstacleData = require(obstacle.ObstacleInfo)
local obstHealth = obstacleData.Health
local blockHealth = self.Health
if blockHealth > obstHealth then
obstacle:Destroy()
self.Health = self.Health - obstHealth
elseif blockHealth < obstHealth then
obstacleData.Health = obstacleData.Health - blockHealth
self:Destroy()
elseif blockHealth == obstHealth then
obstacle:Destroy()
self:Destroy()
end
end
end
return BlockClass
Other script (what is this called?):
local blockClass = require(game.ReplicatedStorage.Modules.BlockClass)
local function generateBlock()
local chosenBlock = blockList[math.random(1, #blockList)]
local block = blockClass.new(chosenBlock, CFrame.new(91.5, 25, -30.5))
if block then
block.Model.Hitbox.Touched:Connect(function(hit)
print(hit)
end)
end
end
I think that a lot of things in this code is wrong. It seems too long and I feel like some things could be better. If you see anything that’s incorrect, please tell me. I was also wondering if you could make your own events using OOP. For example, block.Destroyed:Connect(function().
The thing about OOP is a lot of it comes down to how you like to do things. It’s basically just the idea that you can have things encapsulated in objects & use objects, and it seems here like you’re doing that.
That being said, I’d suggest against using “block.Model.” from your other script (client?). If you change something in your object, you’ll break that script. So I usually like to do it where object properties are accessed through Get/Set operations - so you know who accesses what. In this case, if you need access to Model, I’d be putting in a :GetModel() method.
The better solution here, though, would be that last point you mentioned. The way to do that is with BindableEvents; if you set something up like blockData.Destroyed = Instance.new(“BindableEvent”), then that can be used like a typical event (albeit through the .Event property).
Then the other thing you mentioned is that the code is too long; the way I usually deal with that is by setting up additional functions. If you split up the initialization part into tinier components (InitModel, Start, etc., for example) then you can make it more readable that way. Still long (in terms of lines of code) but you’d know what’s going on.
Your constructor has a lot of if (x) then return end lines. A constructor shouldn’t return a nil value ever (by practice), so I would recommend throwing an error instead. The reasoning is straightforward: If the constructor isn’t given the correct info to construct the object, then that’s a problem worthy of throwing an error to let the consumer of the constructor know that it did something wrong.
In some instances, you also want to be more explicit with your checks. For instance, you have if cframe == nil..., but it might be better to check the type: if typeof(cframe) ~= "CFrame"... just to verify the inputted type is correct.
So, with both of those things in mind, you might have a small change like such:
if typeof(cframe) ~= "CFrame" then error("Expected CFrame for argument #2") end
You could also use an assertion to do the same thing:
assert(typeof(cframe) == "CFrame", "Expected CFrame for argument #2")
An assertion will throw an error if the condition fails.
I’d follow what Crazyman32 said. Although this is subjective, BlockClass could be simplified to Block since you know that you are creating a class (or maybe from how you organize your ModuleScripts).
When I create classes, I use the constructor to simply set up properties, so the part where you SetPrimaryPartCFrame, you would have to call the Generate method. First things first, it is readable. You know what it will do and how it will do it. The second reason is that it is flexible. You can choose to not generate the block when you don’t need to, or you can do it automatically.
For events, you can utilize BindableEvents to create events for your class. I have it put in the constructor.
Here is my changes:
-- We use BindableEvents to create events
local destroyed = Instance.new("BindableEvent")
local Block = {}
Block.__index = Block
function Block.new(block, parent)
local blockData = {}
local blockModel
for _, uniBlockList in pairs(game.ReplicatedStorage.Blocks:GetChildren()) do
if uniBlockList:FindFirstChild(block) then
blockModel = uniBlockList:FindFirstChild(block)
break
end
end
if not blockModel then return end
if not blockModel:FindFirstChild("BlockInfo") then return end
blockData.Model = blockModel:Clone()
blockData.Info = require(blockModel.BlockInfo)
blockData.Health = blockData.Info.Health
blockData.Destroyed = destroyed.Event -- from what you asked
setmetatable(blockData, Block)
return blockData
end
function Block:Generate(cframe)
if typeof(cframe) ~= "CFrame" then
error("Expected CFrame for argument #1")
end
self.blockData.Model.Parent = self.parent or game.Workspace
self.blockData.Model.PrimaryPart = self.blockData.Info.PrimaryPart
self.blockData.Model:SetPrimaryPartCFrame(cframe)
-- I'm guessing you have to call onStart when the block
-- generates
if self.blockData.Info.onStart then
spawn(self.blockData.Info.onStart)
end
end
function Block:TakeDamage(obstacle)
if obstacle and obstacle:FindFirstChild("ObstacleInfo") then
local obstacleData = require(obstacle.ObstacleInfo)
local obstHealth = obstacleData.Health
local blockHealth = self.Health
if blockHealth > obstHealth then
obstacle:Destroy()
self.Health = self.Health - obstHealth
elseif blockHealth <= obstHealth then
obstacleData.Health = obstacleData.Health - blockHealth
self:Destroy()
end
end
end
return Block
Thanks for the help I’ve applied what you’ve all said to the scripts.
I was wondering if I should handle the bindable events in the class script itself or the script that instantiated it?
Class script:
function Block:Configure(cframe, parent)
self.Model.Parent = parent or game.Workspace
self.Model:SetPrimaryPartCFrame(cframe)
if self.Info.onStart then
spawn(self.Info.onStart)
end
self.Destroyed:Connect(function()
if self.Info.onDestroyed then
self.Info.onDestroyed()
end
end)
self.Destroy:Connect(function()
if self.Info.onDestroy then
self.Info.onDestroy()
end
end)
end
Other script:
local blockClass = require(game.ReplicatedStorage.Modules.BlockClass)
local function generateBlock()
local chosenBlock = blockList[math.random(1, #blockList)]
local block = blockClass.new(chosenBlock)
block:Configure(CFrame.new(91.5, 25, -30.5))
block:TakeDamage(game.Workspace.Game.Obstacles.Rock)
block.Destroyed:connect(function()
if block.onDestroyed then
block.onDestroyed()
end
end)
block.Destroy:Connect(function()
if block.onDestroy then
block.onDestroy()
end
end)
end
The configure method is the same as @Nil_ScripterAlt’s generate method, I just thought configure fit better.
Thanks, one final question (I think this is my last). If I want to access the methods from another script, how would I do that without having to call the constructor?
I want to make it so that when a block gets damaged, it heals all the other blocks. To do this, I plan on using a for loop to go find the other blocks and call the Block:RegenerateHealth() method. This wouldn’t work because the script is not the one that instantiated the block so the methods wouldn’t be accessable.
If you want to access the methods, you can simply require the BlockClass ModuleScript and simply call BlockClass:RegenerateHealth() with its respective parameters (the blocks you want to heal). The only problem I could see with this is if you use the self keyword. Since you use the : syntax, you could be using self in your regeneration code. If you are not using self in the code, you should be using the . syntax (in this case, BlockClass.RegenerateHealth() with its respective parameters). This idea is called a static method, it can be called without the need for creating an instance of a class.
Right now I used the block class from another script to create a block. Now I’m using another script and I am trying to heal the block from that script. The script that I am using to regenerate health is below. Right now I am calling the regenerate health method, but without any reference to the block that I want to heal:
local block = game.Workspace:WaitForChild("Aluminum Block")
local blockClass = require(game.ReplicatedStorage.Modules.BlockClass)
blockClass:RegenerateHealth(10)
I want to write block:RegenerateHealth(10) but block is just a model and doesn’t have the methods in the blockClass. Is it possible to write block:RegenerateHealth(10)? Right now that won’t work but if I modify my script could I get that to work?
In Roblox, you can create an instance of an object from another script, for example a Model. In a different script, you can type Model:GetBoundingBox() and it works even though the original model was instantiated in another script. I want to do this with my own class and wondering how I could do it.
My end goal is to be able to type block:RegenerateHealth(x) from a different script and still have it work.
If you have not found a solution yet, what you could do is have a GUID generated for every object and have a global table i.e. _G.Blocks (_G is bad but I can’t think of another way off the top of my head) that you index with the GUID with a reference to the object as the value.
How you would get that GUID is that the block would contain a StringValue with the GUID or a GUID attribute, the GUID StringValue or Attribute would be created in the constructur(.new).
I’ve applied what you’ve all said to the scripts.