ModuleScript Review

Help and Feedback Code Review
#1

Hello!

I just learned how to use Module scripts and decided to make a module for an existing bit of code I had to check if a player had access to a team and to change it if they did. My goal was to make a module that would check for the ownership of a gamepass or being a privelleged user.

After a couple of hours of trial and error, I managed to make it work. I tried to have the local script change the team with this script:

local function hasAccess()
	local hasGamePass = false
	local isPrivilegedUser = false
	PlayerCheckModule.hasAccess(player, gamePassID, TeamName)
end

TeamChangeButton.MouseButton1Click:Connect(function()
if hasAccess(player, gamePassID) then
		player.Team = game.Teams[TeamName]
		print(player.Name .. " has changed teams.")
	else
		warn("player team change denied")
	end
end)

But I kept getting errors on

PlayerCheckModule.hasAccess(player, gamePassID, TeamName)
end

like Attempted to call a nil value or Attempted to call a boolean value, so I meshed the team change script into the module script.

What I want to know is,

1. Is it clean enough? Could certain areas be polished, or are there some redundancies I was unaware of?
2. How can I separate the team changing script from the gamepass check script? How do I use a module script to get a boolean value?

MODULE

local PlayerCheckModule = {}

local Players = game:GetService("Players")
local MarketplaceService = game:GetService("MarketplaceService")

--TODO: Make gamepasses

PlayerCheckModule.gamepassTable = { --EXAMPLES FOR NOW
	123456789, --Captain
	2468101214, -- First Mate
	13579111315, -- Premium
}
PlayerCheckModule.privilegedUsernames = {
	"hugtrain",
}

PlayerCheckModule.Accepted = false

local function hasAccess(player, gamePassID)
	local hasGamePass = false
	local isPrivilegedUser = false

	if MarketplaceService:UserOwnsGamePassAsync(player.UserId, gamePassID) then
		hasGamePass = true
		print("success")
	else
		warn("Gamepass ownership check failed for player: " .. player.Name)
		print(hasGamePass)
	end

	-- Check if the player's username is in the privileged list
	isPrivilegedUser = table.find(PlayerCheckModule.privilegedUsernames, player.Name) ~= nil
	print(isPrivilegedUser)
	print("checked")
	
	return hasGamePass or isPrivilegedUser
end
	
function PlayerCheckModule.changeTeam(player, gamePassID, TeamName)
	print(player, gamePassID, TeamName)
	if not player or not player.Character then return end

	if hasAccess(player, gamePassID) then
		player.Team = game.Teams[TeamName]
		print(player.Name .. " has changed teams.")
	else
		warn("player team change denied")
	end
end

return PlayerCheckModule

LOCAL SCRIPT EXAMPLE

local TeamChangeButton = script.Parent -- Reference to the ImageButton instance

local Players = game:GetService("Players")
local player = Players.LocalPlayer

local MarketplaceService = game:GetService("MarketplaceService")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local PlayerCheckModule = require(ReplicatedStorage.Modules.PlayerCheckModule)

-- Configuration
local gamePassID = 12345678 -- Replace with Game Pass ID
local TeamName = "Captain" -- Replace with team name

local function hasAccess()
	local hasGamePass = false
	local isPrivilegedUser = false
	print(player, gamePassID, TeamName)
	PlayerCheckModule.changeTeam(player, gamePassID, TeamName)
end

TeamChangeButton.MouseButton1Click:Connect(function()
	hasAccess()

end)

Thanks for your feedback!

1 Like
#2

In the hasAccess function, you’re not handling errors properly. If the MarketplaceService:UserOwnsGamePassAsync call fails, it will propagate the error. Consider using pcall to handle errors

#3

I’d like to begin by saying the following, more as something I’ll ignore because it looks like a slight mistake, you have duped hasAccess into your LocalScript example, so uhh, I’ll omit that.

That being said, I have some tips for you. Your module seems to be stateless (It doesn’t hold any variable that might change across functions and alter its behaviour), therefore I’d recommend you to when returning PlayerCheckModule you run table.freeze(…) on it to avoid it being modified, it is just a guarantee. On other notes, I’d recommend you to define your functions at the lowest scope you need them at. For example, take a look at your hasAccess function on the MODULE.

local function hasAccess(player, gamePassID)
	local hasGamePass = false
	local isPrivilegedUser = false

	if MarketplaceService:UserOwnsGamePassAsync(player.UserId, gamePassID) then
		hasGamePass = true
		print("success")
	else
		warn("Gamepass ownership check failed for player: " .. player.Name)
		print(hasGamePass)
	end

	-- Check if the player's username is in the privileged list
	isPrivilegedUser = table.find(PlayerCheckModule.privilegedUsernames, player.Name) ~= nil
	print(isPrivilegedUser)
	print("checked")
	
	return hasGamePass or isPrivilegedUser
end

In it, you have defined isPrivilegedUser at the top, but it is not needed yet, I would recommend you to move it to where you first set it, mostly for the sake of maintaining the code cleaner and remove that from your head while reading it, so you can better focus on it. With that said, someone already pointed it out for you, but functions which involve networking, such as MarketplaceService::UserOwnsGamePassAsync may result in errors due to their nature, I would recommend you to wrap it in a pcall to avoid the possible effects of it, aside from that, I’d like to add that if you know what your function will be taking in, annotate it, this will actually in some cases help your code run faster, since the Luau compiler will be able to compile your bytecode more efficiently, in this case, you can set your player argument to be of type Player, and your gamePassId argument to be a number and your return to be of type boolean. Below is a slightly rewritten version of the code with the changes I propose, I’d recommend you to not take it like this, though, and to make some changes to it for your own purposes.

local function HasAccessToFeature(player: Player, gamepassId: number): boolean
        local success, ret = pcall(function(player: Player, gamepassId: number) return MarketplaceService:UserOwnsGamePassAsync(player.UserId, gamepassId) end, player, gamepassId) -- Pass the arguments into pcall to not capture them.
       
        local hasGamepass = success and ret -- Here we assume that if the function fails, the player does not have the gamepass, but if we do succeed, we also check the parameter to get everything done at once, this is just a short hand for a bunch of if spaghetti.

       -- Why not use the name? The name can be changed, and can break the whole script. Using User ids prevents this. 
       local isPrivilegedUser = table.find(PlayerCheckModule.privilagedUserids, player.UserId) ~= nil
       
       return isPrivilegedUser and hasGamepass
end