My attempt at OOP

Help and Feedback Code Review
#1

Hello, I have recently been trying to shift over into a more OOP style of coding in roblox and I have come up with a base structure which I would like to know your opinions on. It works as I have been making a test game the past couple days trying to use this style but there are some issues I am having which I will mention below. The code I am about to show is just something I made up to show how I have set it up.

Module that “initializes”

local setup = {}
setup.__index = setup

function setup:Init()
	for _, mod in pairs(script.Parent:GetChildren()) do
		if mod ~= script and mod:IsA("ModuleScript") then 
			setmetatable(require(mod), setup)
		end
	end
end

return setup

Module which creates the object

local mod1 = require(script.Parent.Init)
mod1.__index = mod1

function mod1.newObj(killOnTouch)
	local obj = {}
	setmetatable(obj, mod1)

	local part = Instance.new("Part")
	part.Anchored = true
	part.Color = Color3.fromRGB(0,0,255)

	obj.Part = part
	obj.CanKill = killOnTouch
	if obj.CanKill then obj:ConnectKillFunction() end

	return obj
end

return mod1

Module which has events for the object

local mod2 = require(script.Parent.Init)
mod2.__index = mod2

function mod2:ConnectKillFunction()
	local part = self.Part
	local c = part.Touched:Connect(function(hit)
		if game.Players:GetPlayerFromCharacter(hit.Parent) then
			hit.Parent.Humanoid:TakeDamage(100)
		end
	end)

	self.KillEvent = c
end

function mod2:ChangeColor(color)
	self.Part.Color = color
end

return mod2

Script that calls the initial module and also creates the object and changes its color

local Settings = require(script.Parent.Init)
Settings:Init()

local block = Settings.newObj(true)
block:ChangeColor(Color3.fromRGB(255,0,0))

The end result should be a part which is red and kills on touch

Now the problem I have encountered is I am not sure how to call functions only in modules that have the function. To better explain lets say there is a third module with the code below:

This module sets up a player added function

local mod3 = require(script.Parent.Init)
mod3.__index = mod3

function mod3.Setup()
	game.Players.PlayerAdded:Connect(function()
		mod3.newObj(false)
	end)
end

return mod3

This is the new initialization module which does not work

local setup = require(script.Parent.Init)
setup.__index = setup

function setup:Init()
	for _, mod in pairs(script.Parent:GetChildren()) do
		if mod ~= script and mod:IsA("ModuleScript") then 
			local tab = require(mod)
			setmetatable(tab, setup)
			if tab.Setup then tab.Setup() end --basically i want to call the setup functions in modules that have it, but if they dont have it then nothing happens
		end
	end
end

return setup

I tested this and another way earlier and it ended up with the game freezing so I am not sure how I would go about this. Another solution is to manually call the setup function for the specific modules but I feel like their might be a better way which I am missing.

I am very new to this so I am sorry if something I am doing is really bad but thanks anyways for reading!

2 Likes
#2

Try using a wait() on each of those scripts, usually though there will be reasons of why things aren’t going the way you want them to in the “Output”

1 Like
#3

I would like to avoid using waits to solve problems. Also the error mentioned something about “stack overflow”.

2 Likes
#4

Yeah, add wait() to each of the scripts.
And just wait(), it will only delay it by 0.05
It is much better then a game crashing.

1 Like
#5

I dont think the problem is there is no wait. If I comment out that one line the script works fine. I believe it may have to do with some sort of infinite loop but Im not sure. I am basically asking what is the correct way to do what I said above if that makes sense.

1 Like
#6

The problem isn’t about wait(), and doing that is probably more harm than it is good here. If your problem is fixed by just adding wait() also, you’re probably doing something wrong.

As for @edman101902, the issue seems to be that you have circular dependencies. Your setup:Init (which should be setup.Init, because it’s not a method since self has no practical use there) requires all modules parented to its same parent. This includes your mod3 module. The mod3 module, then requires your setup module too. Since neither is finished in execution, you end up with a circular dependency that never ends.

A solution might be to have your modules handle the require and setup in their .Setup function. For example, you can do:

local mod3 = {}
local mod1

function mod3.Setup()
    mod1 = require(script.Parent.Module1)
    game:GetService('Players').PlayerAdded:Connect(function()
        mod1.newObj(false)
    end)
end

You shouldn’t be using Init as your “main” of sorts like you currently do. Every script would be requiring and modifying the same table, which leads to a huge mess of weird writes. Each module should be requiring only the other modules that it needs, and only returning the table made within it.

That is to say, don’t do function mod1.newObj(killOnTouch) in your Module1 if mod1 is just the result to require(script.Parent.Init). You should create a new table instead.

4 Likes
#7

Ok that makes sense. So what is a better way the Init module should be used?

1 Like
#8

I admire your devotion to the language and to strive further. Object Oriented Programming in Lua is very optional. Most people like to use OOP to have a more structured and readable code. However, some people don’t use OOP for different reasons such as, more indexing, more work, etc. Personally I’d only implement any relating styles of the imperative paradigm (including OOP) in more complex and lengthier programs in Lua. Otherwise I’d stick to traditional functional programming. Overall, your structure looks decent however their are a few bad practices I see that I should comment on.

  • Wrong iterator, you should consider using ipairs on your first section of code. Instances serve as number-type references to a table so Ipairs should be used here as ipairs are usually faster then pairs performance wise.

  • Unneeded indexing, on your second section of code,

      function mod1.newObj(killOnTouch)
      local obj = {}
      setmetatable(obj, mod1)

      local part = Instance.new("Part")
      part.Anchored = true
      part.Color = Color3.fromRGB(0,0,255)

      obj.Part = part
      obj.CanKill = killOnTouch
     if obj.CanKill then obj:ConnectKillFunction() end

      return obj
  end

    I recommend doing this like this instead,

    function mod1.newObj(killOnTouch)
  local part = Instance.new("Part")
  part.Anchored = true
  part.Color = Color3.fromRGB(0,0,255)
   
  return setmetatable({
       ["Part"] = part,
       ["CanKill"] = killOnTouch
  },mod1)
end

return mod1

    This is more clean and optimal as there’s less indexing and less scope referencing.

  • Variable naming. Generally good variable names are what improve readability significantly. This doesn’t seem to be a problem much for you however this is not what im a fan of,

      local c = part.Touched:Connect(function(hit)

    This practice isn’t that bad in this specific case since your program was straightforward however, good variable naming should be accounted for in large complex programs.

Overall, your script looks great. I couldn’t get the full context behind what your trying to do so I can’t judge you on your problem solving skills. Please reply to me if you disagree with any of my criticism!

2 Likes