Custom Class/Instance

Help and Feedback Code Review
#1

While experimenting with custom classes with metatables I made a somewhat simple Package class (literally like envelopes and packages that you deliver)

I just feel like the script could be cleaner, any idea’s on how I could clean it up some more?

local PackageModule = {}
local Packages = {}
local PackageClass = {_ratio = {1, 1, 1, 2}}
local CreatePackage, ObjectManager, PlayerData
local HttpService = game:GetService("HttpService")

function PackageClass:Deliver()
	
end

function PackageClass:Claim()
	local Player = self.Ownership
	local PlayerStorage = PlayerData[Player].Storage
	if PlayerStorage.UsedStorage + 1 >= PlayerStorage.MaxStorage then
		PlayerStorage.UsedStorage = PlayerStorage.MaxStorage
	else
		PlayerStorage.UsedStorage = PlayerStorage.UsedStorage + self.Level
	end
end

function PackageClass:Destroy() -- Destroy a package
	if Packages[self.Ownership] then
		for index,packageData in pairs(Packages[self.Ownership]) do -- Cycle through all packages checking for proper ID
			if self.ID == packageData.ID then
				self.Object:Destroy()
				table.remove(Packages[self.Ownership], index) -- Removing package
			end
		end
	end
end

function PackageModule.New(Player) -- Creating a Custom Instance using the Metamethod of __index = PackageClass
	local ID, Level = tostring(HttpService:GenerateGUID(false)), PackageClass._ratio[math.random(1, #PackageClass._ratio)] -- Id, Level
	local Package = { -- Game Framework requires Object Services to be within a metatable
		Object = ObjectManager.New({
			Name = ID,
			Type = "Package", -- Type is Package, special settings for it
			Player = Player,
			Level = Level,
		}),
		Level = Level, -- May be deprecated! Random level selection of the package
		ID = ID,
		Ownership = Player,
	};
	setmetatable(Package, {
		__index = PackageClass -- Custom Class/Instance now!
	});
	Packages[Player] = Packages[Player] or {}
	table.insert(Packages[Player], Package) -- Inserting the Package
	return Package
end

function PackageModule.Delete(Player) -- Cleanup / Preventing Memory leaks for when a user leaves
	local PlayerPackages = Packages[Player]
	if PlayerPackages then
		PlayerPackages = nil -- Deleting table
	end
end

function PackageModule:Start()
	ObjectManager = self.Modules.ObjectManager
	PlayerData = self.Modules.PlayerData
end

return PackageModule

Or do you think this is good?

5 Likes
#2

I’m not the bestest person when it comes to code optimization and cleanup.

But either way, I can assure you that your code looks pretty neat, easy to understand and clean!
I like how you have added comment lines in your code, if there’s anything that’s not so easy to understand, your comments help.

Good job!

#3

Looks neat.

The only thing I could possibly suggest is using some in line if statements ;

if not Packages[Player] then -- Checking if the player has a package table
		Packages[Player] = {} -- Creating one
	end

--To;

Packages[Player] = Packages[Player] or {} -- create new if doesn't exist.
1 Like
#4

I completely forgot that is something is nil it will go to the next condition
I modified the script and updated the post, thanks!

2 Likes
#5

This is a bit off-topic, but I’d recommend moving your post to #development-support:requests-for-code-feedback rather than scripting support.

#6

This part actually does nothing because PlayerPackages is a local variable to this scope. What you should be doing instead is Packages[Player] = nil to reference the actual value in the table instead.