Basic OOP - Game Menu

[mc3334]

I am fairly new to OOP, to practice the concepts I decided to make a little menu where each button has its own object. Any critiques?

local buttons = {}

function buttons.new(name,displayOrder,callback)
	local self = setmetatable({},{__index = buttons})
	
	--Static
	self.name = name
	self.displayOrder = displayOrder
	self.callback = callback
	--Dynamic
	self.reference = newproxy()
	
	return self
end

function buttons:initialize(parent)
	local button = Instance.new("TextButton")
	button.BackgroundColor3 = Color3.fromRGB(99, 99, 99)
	button.LayoutOrder = self.displayOrder
	button.Name = self.name
	button.Size = UDim2.new(1,0,0,50)
	button.Font = Enum.Font.Fantasy
	button.Text = self.name
	button.TextSize = 25
	button.TextColor3 = Color3.new(1,1,1)
	button.Parent = parent
	
	self.reference = button
end

function buttons:onclick()
	self.callback()
end

return buttons

---

local buttonParent = script.Parent.MainMenu

local buttons = require(script.Buttons)

local buttonIndex = {
	{name="Play",callback=function()
		print("Play")
	end},
	{name="Pause",callback=function()
		print("Pause")
	end},
	{name="Stop",callback=function()
		print("Stop")
	end},
	{name="Settings",callback=function()
		print("Settings")
	end},
	{name="Help",callback=function()
		print("Help")
	end},
	{name="Credits",callback=function()
		print("Credits")
	end},
	{name="Exit",callback=function()
		print("Exit")
	end}
}

local allButtons = {}

for index,buttonData in pairs(buttonIndex) do
	table.insert(allButtons,index,buttons.new(buttonData.name,index,buttonData.callback))
end

for index,buttonObject in pairs(allButtons) do
	buttonObject:initialize(buttonParent)
	buttonObject.reference.MouseButton1Click:Connect(function()
		buttonObject:onclick()
	end)
end

[colbert2677 (Dreamseeker)]

I feel like this is one of the stranger use cases for stateful objects. Simply, you really don’t need this pattern here and this isn’t an ideal test environment to try your hand at objects either. I call it stateful but that’s probably not correct anyway because this class doesn’t hold state and I’m not calling it OOP mainly because it’s not following the principles of OOP.

An OOP-in-Lua class will typically follow this format:

local Class = {}
Class.__index = Class

function Class.new()
    local self = setmetatable({}, Class)

    return self
end

return Class

The callback portion is weirdly made because you can just connect to the function directly. Instead of an onclick method, I would suggest just setting the callback field directly. Then in MouseButton1Click, you can pass the function directly over an anonymous function.

buttonObject.reference.MouseButton1Click:Connect(buttonObject.callback)

Good start for making an easy way to handle menu buttons though without hardcoding all of their functions in.

[DarthFS]

Here is a revamped version -

→ Module

local interface = script.Parent.Parent.MainMenu

local buttons = {}
buttons.__index = buttons -- better than packing a table everytime the constructor is called.

function buttons.new(name, displayOrder, callback) --> Buttons
	-- i prefix a key with "_" to show it's a private member of the class.
	local self = setmetatable({
		_name = name,
		_displayOrder = displayOrder,
		_callback = callback,
	}, buttons)
	self:init()

	return self
end
function buttons:init() --> Void
	local button = Instance.new("TextButton")
	button.BackgroundColor3 = Color3.fromRGB(99, 99, 99)
	button.LayoutOrder = self._displayOrder
	button.Name = self._name
	button.Size = UDim2.new(1,0,0,50)
	button.Font = Enum.Font.Fantasy
	button.Text = self._name
	button.TextSize = 25
	button.TextColor3 = Color3.new(1,1,1)
	button.Parent = interface

	self._button = button -- "reference" is pretty broad since all variables are referenced.
end
function buttons:MouseButton1Click() --> RBXScriptConnection
	return self._Button.OnMouseButton1Click:Connect(self._callback)
end
return buttons

→ Implementation

local buttons = require(script.Buttons)

local buttonIndex = {
	{name="Play", callback=function()
		print("Play")
	end},
	{name="Pause", callback=function()
		print("Pause")
	end},
	{name="Stop", callback=function()
		print("Stop")
	end},
	{name="Settings", callback=function()
		print("Settings")
	end},
	{name="Help", callback=function()
		print("Help")
	end},
	{name="Credits", callback=function()
		print("Credits")
	end},
	{name="Exit", callback=function()
		print("Exit")
	end}
}
local allButtons = {}

for index, data in pairs(buttonIndex) do
	local buttonObject = buttons.new(data.name, index, data.callback)
	buttonObject:MouseButton1Click()	
	
	table.insert(allButtons, buttonObject)
end

• Just like @colbert2677 mentioned your onclick method has unnecessary nested function calls. Same applies to your implementation where you setup the MouseButton1Click connection.
• I added more abstraction to make the module more user-friendly / easier to implement.
• No unnecessary memory footprint (self._reference = newproxy()).
• My implementation prevents having to iterate through a table 2 times.

*Note that that prefixing the key of an object with “_” is totally optional. It’s just my preference when making a class.

[utrain]

Whitespace

Whitespace is *very* important for the readability of your code. Without it, the text will look crowded, and will be harder to parse through the text. Here's some recommendation on whitespace.

1. Add space after the comma of a field, parameter, operations, the start of a comment, or argument.
2. Add a new line after every field, parameter, operation, or argument if the line would be too long. It’s also important to make your judgment if the line’s readability would be helped by splitting it into several more. I recommend you set the limit to be around 80 characters. However, the limit you set is really up to you to decide. I know people that use 160, 120, 90, etc.

Example

-- OK!
b = function(a, b, c) 
    return {
        a + b, 
        (c + b) ^ 2 
    }
end 
-- I THINK IT'S OK
c = { 1, 2, 3, 4 } -- Not too long and conise.

3. Split your code into relevant chunks using newlines.

Example (With your code)

Note the addition of an empty line.

	-- Static
	self.name = name
	self.displayOrder = displayOrder
	self.callback = callback

	-- Dynamic
	self.reference = newproxy()

3. Add new lines to functions.
4. Have consistent whitespacing (and well, code style) overall.
5. There’s more to find, do some research on this!

Initialize in `.new`!

There’s a problem, you have a self.reference field, but it’s set properly after calling initialize. Meaning, .new object doesn’t actually fully construct the object.

.new is expected to have the object already initialized and working. So, either invoke initialize in your .new constructor if you expect it to be overridden (this implies this is an abstract class that needs to be inherited from) or simply move the logic into .new. In either case, you’ll need to create a new parameter, parent, from the .new function.

local buttons = {}
buttons.__index = buttons --@colber2677

function buttons.new(name, displayOrder, callback, parent)
	local self = setmetatable({}, buttons)
	
	-- Static
	self.name = name
	self.displayOrder = displayOrder
	self.callback = callback

	local button = Instance.new("TextButton")
	button.BackgroundColor3 = Color3.fromRGB(99, 99, 99)
	button.LayoutOrder = self.displayOrder
	button.Name = self.name
	button.Size = UDim2.new(1,0,0,50)
	button.Font = Enum.Font.Fantasy
	button.Text = self.name
	button.TextSize = 25
	button.TextColor3 = Color3.new(1,1,1)
	button.Parent = parent
	
	self.reference = button
	
	return self
end

Callbacks

Callbacks are good.

However, the way it has been done thus far, these two…

@DarthFS

function buttons:MouseButton1Click() --> RBXScriptConnection
	return self._Button.OnMouseButton1Click:Connect(self._callback)
end

@colbert2677

buttonObject.reference.MouseButton1Click:Connect(buttonObject.callback)

… aren’t the best way.

The reasons are two-fold:

1. You want to be able to dynamically change the callback, by doing the above examples, you will force the field to be permanently set to one callback. Modifying that field will not change as intended.
2. OK, you also want to be able to define your own behavior that may involve that same event. So, now you’re forced to make two connections for no good reason. Moreover, you may want to pass in special argument(s) to the callback (id of the button, the object itself, etc). This is impossible with the current setup.

And, in terms of how DarthFS did it (first example), it forces us to handle the disconnection and connection created. This should be handled by the object instead! We should handle the connection in .new and disconnection I’ll get into later.

Besides that… what to do? We can make a separate function that can handle this for us. I’m going to assume that self.reference is already initialized for ease. Read the last section for that.

function buttons.new(name, displayOrder, callback)
	local self = setmetatable({},{ __index = buttons })
	
	-- ... Skipping lines ...

	self._mouseEvent = self.reference.MouseButton1Down:Connect(function(...)
		-- We can now do something like...
		-- self.activated = true
		-- If desired...

		self.callback(self, ...) -- Or, we don't pass self? We can do whatever we want.
	end

	return self
end

Cleaning up after yourself...

In the end, we all make messes. It can be actually painful to clean up messes. Or... completely impossible depending on how we have done everything. So, let's make it easier by defining a method to do exactly that!

function buttons:destroy()
    -- If we're destroying the object the event is connected to, this isn't needed. 
    -- But, getting point across right now.
	self._mouseEvent:Disconnect() 

	self.reference:Destroy()

	-- And we're done cleaning up!
end

In the future, you’ll find that you’ll be cleaning up large messes. I recommend you take a look into maids to help with this.

Isn't this overkill in the end?

I agree with @colbert2677 here,

@colbert2677

I call it stateful but that’s probably not correct anyway because this class doesn’t hold state and I’m not calling it OOP mainly because it’s not following the principles of OOP.

In the end, this isn’t really doing anything that a simple function couldn’t do. In fact, I bet you can pull your init function and modify some things.

local function createButton(parent, displayOrder, callback)
	local button = Instance.new("TextButton")
	button.BackgroundColor3 = Color3.fromRGB(99, 99, 99)
	button.LayoutOrder = displayOrder
	button.Name = name
	button.Size = UDim2.new(1,0,0,50)
	button.Font = Enum.Font.Fantasy
	button.Text = name
	button.TextSize = 25
	button.TextColor3 = Color3.new(1,1,1)
	button.Parent = parent
    button.MouseButton1Down:Connect(callback) -- I have no special things to do.
	
    return button
end

Just remember, use the right tools for the job at hand.

Some other ones...

• There should be a menu class as well, which should create the menu, and manage the menu and buttons.
• Merge your parameters into one dictionary.

function buttons.new(props) -- Shorthand for properties.
	local self = setmetatable({},{__index = buttons})
	
	--Static
	self.name = props.name
	self.displayOrder = props.displayOrder
	self.callback = props.callback
	-- ...

• Add defaults.

self.name = self.name or "defaultName"
self.displayOrder = self.displayOrder or 1
-- ...