OOP is pretty easy to spoil 2: Electric Boogalo

Hi. Recently I posted here asking for tips on OOP classes for events. Was suggested using metatables by a lot of people so I rewrote my module with them.
However, is it just me or it looks… more dirty now and unnecessarily overcomplicated? Here’s what it looks like right now:

-- Base class
BaseEvent = {}
BaseEvent.__index = BaseEvent
function BaseEvent.new(name, description)
	local Event = {}
	setmetatable(Event, BaseEvent)
	
	Event.DisplayName  = name         or "BaseEvent"
	Event.Description  = description  or "BaseDescription"

	return Event
end

function BaseEvent:Run()
    print(self.DisplayName .. " is called.")
end

-- Unique event
Fireworks = {}
Fireworks .__index = Fireworks 
setmetatable(Fireworks , BaseEvent)
function Fireworks.new()
	local Event = BaseEvent.new("Fireworks", "Nice stuff going off")
	setmetatable(Event, Fireworks)		
    Event.FireworkColors = ColorSequence.new(Color3.new(1,0,0), Color3.new(0,0,1))
	return Event
end	
	
function Fireworks:Run()
	BaseEvent.Run(self) -- Call base event
    --Unique code goes here that uses self.FireworkColors--
end

As you can see, the constructor for Fireworks is pretty messy. It would be this way for every event since all of them would have their unique parameters. Any ways I can clean this up?

My goal here is to get events from any place in the game and have them as instance. Ideally, I would get events from this module as:

--Here I just run an event
Events.Fireworks.new():Run()

--Here I run an event but keep it as an instance for later usage. 
local event = Events.SomeOtherEvent.new()
event:Run()
print(event.ElapsedTime)
1 Like

That’s right. But I can’t decide on the model I should go for. Essentially I want a system that:

  • Returns an instance
  • Polymorphs
  • Doesn’t require multiple module scripts (I really just want to keep all my events in a single script)

Can you elaborate what you mean by “a single script call the module function”? as in
Events:Fireworks(parameters) in my case?

Modules cache their return value after being required once.

This is a classic OOP structure he’s using, and I don’t really see how this is overcomplicated. It’s very similar to the one used by PiL, so it’s not unheard of to structure it this way.

It only looks unnecessarily complicated because there’s not much code in your class yet. Once your class gets some non-trivial implementation in it that 4 lines of boilerplate will feel a lot less important.

You can write some kind of system to encapsulate that behavior and “look” nicer but:

  1. You lose the ability of anyone who reads the code (including future you) to easily and quickly understand what you’re doing.

  2. You lose the ability to just take this code with you anywhere you need it, because now that abstraction layer has to come along with it.

  3. It’s a small enough amount of boilerplate that you won’t really improve things by abstracting it, you’ll just make them look more “clever”.

TL;DR: I think the code is good as is.

3 Likes

Thanks, I thought about it and it looks like this is as good as this model can get without losing functionality or spending too much time developing and optimizing it (without much benefit). Just wanted to know whether it’s something acceptable when working with lua’s oop :smiley: