How good is my class?

Help and Feedback Code Review
#1

I’ve made a similar post a couple months ago and wondering if there is room for improvement but I’ve never gotten a response. I’m trying once again.

local Signal = require(script.Signal)
local Noob = {}

local mt = {}
mt._events = {}

function Noob.new(health: number?, name: string?)
	local properties = {
		Health = health or 100,
		Name = name or "Bob"
	}
	
	for property, value in pairs(properties) do
		mt._events[property] = Signal.new()
	end
	
	mt.__newindex = function(t, k, v)
		--print("User has indexed " .. k .. " with " .. v .. " | __newindex has been invoked")
		
		if properties[k] then			
			rawset(properties, k, v)

			if mt._events[k] then -- if a signal exists for that property
				mt._events[k]:Fire(v)
			end
		else
			warn("[NoobClass] You cannot set new values into this Class")
		end
	end
	
	mt.__index = function(t, k)
		--print("User has indexed " .. k .. " | __index has been invoked")
		local property = rawget(properties, k)
		if property then return property end
	end
	
	mt.__tostring = function(t)
		return "NoobClass"
	end
	
	return setmetatable(Noob, mt)
end

function Noob:GetPropertyChangedSignal(property, listener)
	local signal = mt._events[property]
	if signal then return signal end
end

return Noob
1 Like
#2

One potential improvement for this script would be to add additional methods to the Noob class to allow users to perform actions on the instances of the class. For example, you could add a TakeDamage method that would decrease the instance’s Health property by a specified amount.

#3

Here is an improved version of the script:

local Signal = require(script.Signal)
local Noob = {}

local mt = {}
mt._events = {}

function Noob.new(health: number?, name: string?)
	local properties = {
		Health = health or 100,
		Name = name or "Bob"
	}

	mt.__newindex = function(t, k, v)
		--print("User has indexed " .. k .. " with " .. v .. " | __newindex has been invoked")

		if properties[k] then			
			rawset(properties, k, v)

			if mt._events[k] then -- if a signal exists for that property
				mt._events[k]:Fire(v)
			end
		else
			warn("[NoobClass] You cannot set new values into this Class")
		end
	end
	
	mt.__index = function(t, k)
		--print("User has indexed " .. k .. " | __index has been invoked")
		local property = rawget(properties, k)
		if property then return property end
	end
	
	mt.__tostring = function(t)
		return "NoobClass"
	end
	
	return setmetatable(Noob, mt)
end

function Noob:GetPropertyChangedSignal(property, listener)
	local signal = mt._events[property]
	if signal then return signal end
end

return Noob

Here are the changes I made:

  • I removed the for loop that iterated over the properties table and created a signal for each property. This loop was unnecessary because the __newindex metamethod already handles the creation of signals for properties when they are set.
  • I removed the unused listener argument from the GetPropertyChangedSignal function.
#4

I don’t think the event is going to work as intended because you are using a constant metatable. Try creating two objects:

local noob1 = Noob.new()
local noob2 = Noob.new()

After that, connect both to GetPropertyChangedSignal method, then try changing one of the properties of noob1; you’ll notice that it also fires noob2’s property and changes it to whatever value you set.

A quick fix would most likely be to create a metatable within the scope of the new method.

Edit 1: I just tried your script, and my hypothesis is correct: it does fire both noob1 and noob2’s properties:

local Noob = require(script.Parent.Noob)

local noob1 = Noob.new(nil, "Noob1")
local noob2 = Noob.new(nil, "Noob2")

noob1:GetPropertyChangedSignal("Name"):Connect(function(...)
	print(..., noob1.Name)
end)

noob2:GetPropertyChangedSignal("Name"):Connect(function(...)
	print(..., noob2.Name)
end)

noob1.Name = "Bob1"

Edit 2: This is what it should look like, I just tried this one and works perfectly does not fire the other noob object’s event.

local Signal = require(script.Signal)
local Noob = {
	__tostring = function()
		return "NoobClass"
	end,
}

function Noob:__index(index)
	local property = self.__properties[index]

	if property then
		return property
	end

	local method = Noob[index]

	if method then
		return method
	end

	error(('%s is not a valid member of NoobClass "%s"'):format(index, self.__properties.Name))
end

function Noob:__newindex(index, value)
	local properties = self.__properties
	local propertyType = typeof(properties[index])

	if propertyType then
		if typeof(value) == propertyType then
			properties[index] = value
			return self.__events[index]:Fire(value)
		end

		error(("Unable to assign property %s. %s expected, got %s"):format(index, propertyType, typeof(value)))
	end

	error(('%s is not a valid member of NoobClass "%s"'):format(index, self.__properties.Name))
end

function Noob.new(health: number?, name: string?)
	local self = setmetatable({
		__properties = {
			Health = health or 100,
			Name = name or "Bob",
		},

		__events = {}
	}, Noob)

	for property, _ in pairs(self.__properties) do
		self.__events[property] = Signal.new()
	end

	return self
end

function Noob:GetPropertyChangedSignal(property)
	local signal = self.__events[property]
	if signal then return signal end
end

return Noob
1 Like