Code Smell or Bad Practice

So I’m working with OOP and I was wondering if this is “bad practice”.

Parent class:

local Objective = {}
Objective.__index = Objective

function Objective.new()
    local self = {}
    --something happens (this doesn't happen upon instantiation")
    self:UpdateBillboard() --not in the "Objective" class, actually defined later in object
    return self
end

Object of class:

local Repair = {}
Repair.__index = Repair
setmetatable(Repair, Objective)

function Repair:UpdateBillboard() --this is actually used by the Parent Class, "Objective"
    --do something
end

function Repair.new()
    local self = Objective.new() --inherit everything from "Objective" parent class
    setmetable(self, Repair)
    return self
end

Is the Objective class using the method “UpdateBillboard” method from an object under its class bad?

4 Likes

Using a method of a base class where it’s not implemented in the base class is a common OOP pattern – it’s called an abstract class and an abstract method. In this case, the abstract class is Objective.

In most strict OOP languages, you’re not allowed to instantiate (create) the base abstract class (Objective), you can only create its non-abstract subclasses (Repair).

This is practically the whole point of inheritance and subclassing. You can know that an object has a method without knowing what it does.


The only part I might find questionable about this is Objective calling one of its own abstract methods. I’ve never seen any advice against this and I couldn’t find any advice against it with a quick google search. I’m pretty sure that’s common practice, and I’m pretty sure I’ve done it myself. It’s good!

1 Like

Yeah, the reason why I didn’t have UpdateBillboard method under the Objective class is because there’s multiple types of Objective sub-objects, and they don’t all have the the same UpdateBillboard internals, but they do all fire this method by name given the same conditions.

This is how I found myself in this type of situation.

This is normal OOP practice. If you couldn’t define methods with different behavior in subclasses, then what would be the point of inheritance or abstract methods?

When you need a set of objects that have different behavior under the same conditions, the OOP way to do it is to create a base class and extend it with subclasses that override behaviors that need changing. It’s fine if the base class doesn’t actually include the method, as long as you never create the base class directly and as long as you make note that its subclasses have the method.

5 Likes

The reason why I create the base class is because all the objects share x amount of default properties of it.

That’s also a fine reason to create a subclass.

One OOP reason to create a subclass is so you have objects that you know have the same methods, even if they have different behaviors.

Another OOP reason to create a subclass is so you can reuse methods and properties from the superclass. In other words, so that you can have default methods or default properties.

In this case you’re doing it for both reasons! All good :+1:


Outside of principles, I don’t know if the given code will actually work. I’m hoping it’s just example code.

It won’t work because you need the Repair metatable to call UpdateBillboard, but you add in the Repair metatable after you call UpdateBillboard.

I would suggest having separate new and Construct methods, and using some helper functions. Here’s a short example:

local baseClass = {}
baseClass.__index = baseClass

function baseClass:AbstractMethod()
	error('Cannot call abstract method')
end

local function Class(super)
	local class = setmetatable({}, super or baseClass)
	class.__index = class
	class.isAbstract = false
	function class.new(...)
		if class.isAbstract then
			error('Cannot create abstract class')
		end
		local self = setmetatable({}, class)
		self:Construct(...)
		return self
	end
	return class
end

---

local Objective = Class()
Objective.isAbstract = true -- prevent creating Objectives directly

function Objective:Construct()
	self:UpdateBillboard()
end

Objective.UpdateBillboard = Objective.AbstractMethod -- prevent calling UpdateBillboard directly until overridden

---

local Repair = Class(Objective) -- automatically sets isAbstract = false

function Repair:Construct()
	Objective.Construct(self) -- you don't actually need this: if you don't have a Repair:Construct, it'll just use Objective:Construct
	print('Yay! I exist!')
end

function Repair:UpdateBillboard()
    print('Yay! You can call me now!')
end

---

Repair.new() -- works

Objective.new() -- errors: Cannot create abstract class
5 Likes

yeah thats why i said

I know it’s dangerous, though. It should be all good as long as I call methods after instantiation, right?

Another way I would do it is have a placeholder method in the parent class, and then have the sub objects override it by setting their own methods with the exact name.

If you only call subclass methods after instantiation, then yeah it should be fine.

You have to be really careful though! Let’s say, for example, during instantiation you add an Instance to some folder – let’s say “PlayerData”. Then another script picks up that ChildAdded event, and triggers code in your yet-to-be-finished-instantiating object! You have to be careful of every single move you make during instantiation!

This could happen with any Roblox event, and you could accidentally cause this through regular function calls if you aren’t careful enough. If it happens because of a Roblox event, you won’t even get a traceback to the source if it errors! (You’ll only get one back to the event function, not to what triggered the event, which is what matters.)

If both the superclass and subclass have valid, callable methods then you can end up with the superclass method being called when you expect the subclass method to be called. This will result in the game state not being what you expect, and result in hard-to-find, time-consuming bugs!

In my opinion, there’s no reason to deal with that when a helper function or two can remove that whole headache and make your code easier to read and write. An important part of programming is planning for your own failures. You can save time in the future by preparing for this now.


If you just have an empty placeholder method, then this won’t fix your issue here, it will only mask it. If the method in the superclass is ideally never supposed to be called, then it being called means that you made a mistake and your code won’t work how you expect. That’s also known as a bug, and it’s much better to catch bugs as they happen so you can fix them rather than let them silently pass on by.

On the other hand, if your placeholder method errors telling you that you’re calling the wrong thing, then that is very helpful!

2 Likes

I also have another question, and it relates back to my original question.

Would it be ok if the super class has a method, but is editing a property only setted by the sub object under that class?

Assuming all the methods are called after 100% proper instantiation.

That would be called an abstract property, and it’s another common OOP practice. The superclass has an abstract property, meaning it can’t/shouldn’t be used, then all of its subclasses can implement that property to make them concrete.

1 Like
local Gun = {}
Gun .__index = Gun 

function Gun:Fire()
    self.ammo = self.ammo - 1
end

function Gun.new()
    local self = {}
    return self
end
local Pistol = {}
Pistol .__index = Pistol 
setmetatable(Pistol, Gun)

function Pistol .new()
    local self = Gun.new()
    setmetable(self, Pistol)

    self.ammo = 10 --because different gun types have different stats

    return self
end

So would something like this be okay?

Yes, that’s reasonable. ammo is the abstract property in this case.

2 Likes

Alright thanks for your help!

I can continue on my project now without being paranoid I’m doing something unconventional. :slight_smile:

1 Like

I do this routinely too. The function can be empty, but if I’m trying to make a Lua-equivalent of what would be a pure virtual method in C++, what I will do is have the base class version just include something like warn("Call to base class method X that should have been overridden!")

2 Likes