I’ve been working with commands for a while now, and I believe there’s nothing I could improve as of right now. Perhaps you guys have something to include.
Any feedback is appreciated when it comes to optimization and performance, as well as possible things to avoid that might not be needed in the script.
This script is only an example. You could call it pseudocode.
local Players = game:GetService('Players')
local Prefix = '/'
Players.PlayerAdded:Connect(function(Player)
Player.Chatted:Connect(function(Message)
local Splits = Message:split(' ')
local Arg1 = Splits[1]
local Arg2 = Splits[2]
local Arg3 = Splits[3]
local Arg4 = Splits[4]
-- Example #1: Getting a specific player
if Arg1:lower() == Prefix .. 'get' and Arg2 then
local GetPlayers = Players:GetPlayers()
for i = 1, #GetPlayers do
local Target = GetPlayers[i]
if Target.Name:lower():sub(1, #Arg2:lower()) == Arg2:lower() then
print(('%s has been found!'):format(Target.Name))
end
end
end
-- Example #2: Doing something more advanced in terms of string.split and string.sub
if Arg1:lower() == Prefix .. 'set' and Arg2 and Arg3 and Arg4 then
local GetPlayers = Players:GetPlayers()
for i = 1, #GetPlayers do
local Target = GetPlayers[i]
if Target.Name:lower():sub(1, #Arg2:lower()) == Arg2:lower() then
local Leaderboard = Player:FindFirstChild('leaderstats')
if Leaderboard then
local LeaderboardValues = Leaderboard:GetChildren()
for i = 1, #LeaderboardValues do
local StatName = LeaderboardValues[i]
if StatName.Name:lower():sub(1, #Arg3:lower()) == Arg3:lower() then
if StatName:IsA('StringValue') and tostring(Arg4)
or StatName:IsA('IntValue') and tonumber(Arg4) then
StatName.Value = tonumber(Arg4) or tostring(Arg4)
-- ^^ Perhaps this logic could be optimized more, not sure at the moment ^^
-- Currently preventing cases where if you change your intvalue to a string, it would convert itself to 0.
-- And at the same time allowing strings to be numbers.
print(("%s's %s has been changed to %s"):format(Target.Name, StatName.Name, tostring(Arg4)))
-- tostring(Arg4) in case the given argument (Arg4) is an integer.
-- Any feedback is appreciated!
end
end
end
end
end
end
end
end)
end)
for i = 1, #GetPlayers do
local Target = GetPlayers[i]
if Target.Name:lower():sub(1, #Arg2:lower()) == Arg2:lower() then
print(('%s has been found!'):format(Target.Name))
end
end
Create an empty table of players and add the player’s name in lowercase as a key, as well as their default case as a key during the PlayerAdded event, and remove it during the PlayerRemoving event.
That way, you can just directly get the player from the table and compare it to Arg2:lower() instead of looping each time the Chatted event runs.
In addition, I would organize your code better and be less repetitive. Getting a specific player isn’t so bad, but the amount of nested if statements as well as the nested for loop makes the second portion pretty unreadable.
I recommend creating a module script to handle reusable functions for each command you make.
I still believe you’d loop through the table you create in order to compare it to Arg2.
Hence: A loop is still required, I don’t really see the need of storing players in a table when you can loop through the online players, in my vision it would just create unnecessary memory.
I was thinking about creating a module for this, and yes - it would save a lot of lines, it would also look a lot cleaner as I could just call (ex): Module:Kick(Target, Reason).
You would not need to loop through the table again to compare it to Arg2. Here’s an example.
PlayersInGame = {
player1 = Player1,
syclya = Syclya
}
local Players = game:GetService('Players')
local GetPlayers = Players:GetPlayers()
local Player = Players[PlayersInGame[Arg2:lower()]] -- This would get the player
The reason I bring this up is because when running commands, you are looping every time, and especially in your second command, you are actually running at O n^2 time complexity every single time, which would be significantly slower.
For the purposes of a small game it would make no difference but it is definitely more time consuming.
This would be efficient with string.sub, as you wouldn’t type their whole name (including capslock) to match it as you did in your example.
But at the same time, you have no control if the player is ingame.
Perhaps add an if statement checking if the player is still in the server.
Anyways, a loop in general is never an issue, it depends what you do inside it, this task alone is no problem and loops like this are no issues, even in the long-run.
On top of that, you would need a .PlayerRemoving event to detect when they leave so you can get rid of their reference in the table (and the memory stored). In my vision there is no need for all this extra work when you can just loop through all players in the server.
Personally, I do not see any issues with my way of referencing the target, I only see it as more work to implement this.
Also, table.find(PlayersOnline, TargetName) would be more efficient because it is faster than indexing (Something[inside]), just a small side-note.
The table I used was an example of how it would look, you would be using string.sub when inserting the key-value pair into the dictionary.
I’m not quite sure what you mean when you say we have no control if the player is in game? PlayerRemoving handles all of that and we’ll just get nil if the player is not in game.
Regardless, it is your choice how you implement your code - just giving my two cents.
Personally, I don’t see any issues with my current way of doing it as there won’t be any errors.
If you don’t mind, please explain your logic on adding this check.
Anyways, to remove the prefix (WHEN NEEDED), gsub works fine in my opinion, I never really needed to remove the prefix as I see no reason to.
Also:
The use of split’s fine , but it’s mostly unnecessary imo.
I kind of disagree, as it was made for stuff like this (commands).
string.split is fairly easy to understand and it makes it very easy for the scripters to learn and understand as it automatically splits arguments for you.
It eliminates having to use Prefix .. "command" for every command that uses it. That way you can get straight to checking what the player said, without having to concat.
if Arg == Prefix .. "kill " then
-- vs
if Arg == "kill " then
It probably was made for use cases that can be for commands, but I don’t believe it’s necessary in this case - it’s not the greatest if you wanted to make an announcement command, without having to use table.concat. That’s just my opinion, however.
I have a lot of commands, and I really don’t see any issues as nothing is hard-coded. Every command uses the same prefix and it works perfectly for my usage.
Overall, if I was going to change the layout, I would probably consider your suggestion, but as of right now I see no need to as my current system is decent for my usage.
The only time I really need to use table.concat is to get the reason for a kick message, etc.