Improvements In Custom Service Code Structure

Help and Feedback Code Review
#1

A few days ago I made a module script to work as a method to streamline using RemoteEvents by adding a channel argument to allow for all network traffic to go through one remote event. If someone can tell me if this is a good idea, please do tell me.

But my main focus was ways to improve my code’s organization. The bullet points below are my main key points I would like to be focused on. Other feedback is welcome as I might have missed things.

  1. Is my CommunicationService:Remove() function properly handling the disposal of my custom object.

  2. Any spots I could use less if statements, especially in my CommunicationService:BroadcastServer() and CommunicationService:BroadcastAllClients() functions?

  3. Use of marking functions / properties that external scripts should not access when using the module. I can easily add functions for external scripts to access those private properties but I removed them to decrease the length of the script present as these functions are very simple and only take one line of code so optimization isn’t a concern.

I’ve noticed I forgot to add a Server > Single Client function. I will add that when I work any revisions.

Code Dropdown 
local runService = game:GetService("RunService")
local IS_SERVER = runService:IsServer()
local remoteEvent = script:WaitForChild("Communication Service Remote Event")

local CommunicationService = {}
CommunicationService.__index = CommunicationService

CommunicationService.DEFUALT_CHANNEL_NAME = "Defualt"
CommunicationService.activeListeners = {}

function CommunicationService:NewListener(channel)
	local channel = channel or CommunicationService.DEFUALT_CHANNEL_NAME
	local newListener = setmetatable({},CommunicationService)
	newListener._customEvent = Instance.new("BindableEvent")

	newListener._activeChannel = channel
	newListener._listenerLocation = CommunicationService._listenerLocation()

	newListener.OnMessageRecived = newListener.customEvent.Event

	table.insert(CommunicationService.activeListeners,newListener)
	return newListener
end

function CommunicationService:Remove()
	self._customEvent:Destroy()
	table.remove(CommunicationService.activeListeners,table.find(self))
end

function CommunicationService:BroadcastServer(...)
	if IS_SERVER then
		warn("BroadcastServer must be called on a Client")
		return
	end

	local args = {...}

	local channel
	local payload
	if #args > 1 then
		channel = tostring(args[1])
		table.remove(args,1)
		payload = args
	else
		channel = CommunicationService.DEFUALT_CHANNEL_NAME
		payload = args[1]
	end

	remoteEvent:FireServer(channel,table.unpack(args))
end

function CommunicationService:BroadcastAllClients(...)
	if not IS_SERVER then
		warn("BroadcastServer must be called on a Client")
		return
	end

	local args = {...}

	local channel
	local payload
	if #args > 1 then
		channel = tostring(args[1])
		table.remove(args,1)
		payload = args
	else
		channel = CommunicationService.DEFUALT_CHANNEL_NAME
		payload = args[1]
	end

	remoteEvent:FireAllClients(channel,table.unpack(args))
end

function CommunicationService:SetDefualtChannelName(channelName)
	if typeof(channelName) ~= "string" then
		warn("Expected string got "..typeof(channelName).." when calling SetDefualtChannelName")
		return
	end
	for _,listener in ipairs(CommunicationService.activeListeners) do
		if listener._activeChannel == CommunicationService.DEFUALT_CHANNEL_NAME then
			listener._activeChannel = channelName
		end
	end
	CommunicationService.DEFUALT_CHANNEL_NAME = channelName
end

function CommunicationService._listenerLocation()
	local locationName
	if IS_SERVER then
		locationName = "Server"
	else
		locationName = "Client"
	end
	return locationName
end

function CommunicationService._serverHandler(player,channel,...)
	for _,listener in ipairs(CommunicationService.activeListeners) do
		if listener._activeChannel == channel then
			listener._customEvent:Fire(player,...)
		end
	end
end

function CommunicationService._clientHandler(channel,...)
	for _,listener in ipairs(CommunicationService.activeListeners) do
		if listener._activeChannel == channel then
			listener._customEvent:Fire(...)
		end
	end
end

if IS_SERVER then
	remoteEvent.OnServerEvent:Connect(CommunicationService._serverHandler)
else
	remoteEvent.OnClientEvent:Connect(CommunicationService._clientHandler)
end

return CommunicationService
#2

I’ll be brief since I don’t feel like dropping an essay →

  1. I’m confused how this actually works. table.find can only accept two arguments.
  1. This function is useless, you already have an IS_SERVER boolean which determines whether or not the script is running server-sided.
  1. The user implementing this module should not have access to these two functions, as they should only be called when fired by the remote event listener that is hooked.

Instead just make these functions not part of the CommunicationService table like so

local function ServerHandler()
    -- Blah
end
local function ClientHandler()
    -- Blah
end
  1. Here is a cool trick you can replace in your :BroadcastAllClients method:
local Array = {1, 2, 3}
local Channel = nil
--> Native Version
Channel = tostring(Array[1])
table.remove(Array, 1)
--> New Version
Channel = tostring(table.remove(Array, 1))
  1. You have a useless payload variable in your :BroadcastAllClients method.
  2. You don’t have a function to fire the remote to one specific client.
  3. When creating an object, you can set the metatable to a separate Client or Server module so you don’t have to worry about whether or not the client is accidentally calling a method meant for the server side.
--> Psuedo Code
setmetatable({}, ( IS_SERVER and ServerM ) or ClientM) --> M Stands for methods
  1. I recommend making your activeListeners table a dictionary instead of an array, so you can index a key in constant time instead of having to iterate. It will also make your code more readable.
  2. This module only accounts for remote events, an doesn’t consider the possibility of remote functions.
  3. Using one remote event to handle all the networking is a terrible idea. Here are a few sources:
    Is there any difference in performance between one single RemoteEvent for all players or a RemoteEvent for each player? - #3 by vsnry
  4. This module doesn’t serve any real purpose compared to utilizing remoteEvents the standard way. Just like @colbert2677 mentioned in a similar thread, it seems like an unnecessary abstraction.
    Remote Module - Public Release - #4 by colbert2677
2 Likes
#3

Thanks the feedback, I never tested the Remove function and had overlooked that issue.

The reason for the listenLocation function was to convert a boolean value to a string value. I did it to help with readability but I agree it could be kept as a bool.

And thank you for pointing out that functions can be excluded from the return table to make them private.

I won’t use this in production games but will make revisions as suggested.

1 Like