I made an admin commands script, but I'm not sure how to test the ban and un ban functions(my friends aren't online and i don't want to accidentally ban them and not have my un ban function work) I'm not sure if I used table.insert and table.remove correctly. Also, i purposely created separate functions for each command, i'm aware i could have used elseif to check if the player said something else. Any tips or tricks would be helpful as well. Thanks in advance. Here's the script:
Admins = {"Player1","LevelKap","Starseus","Personix"} BanList = {} function isPlayerAdmin(name) for _,v in pairs(Admins) do if name == v then return true end end end function KillCommand(msg,rec) if string.sub(msg,1,5) == "kill/" then local player = game.Players:FindFirstChild(string.sub(msg,6)) if player then if player.Character then player.Character.Humanoid.Health = 0 end end end end function KickCommand(msg,rec) if string.sub(1,6) == "kick/" then local player = game.Players:FindFirstChild(string.sub(msg,7)) if player then if player.Character then player:Destroy() end end end end function isPlayerBanned(name) for _, v in pairs(BanList) do if name == v then return true end end end function BanCommand(msg, rec) if string.sub(1,4) == "ban/" then local player = game.Players:FindFirstChild(string.sub(msg,5)) if player then if player.Character then table.insert(BanList,player.Name) end end end end function unBanCommand(msg,rec) if string.sub(1,7) == "pardon/" then local player = game.Players:FindFirstChild(string.sub(msg,8)) if player then if player.Character then table.remove(BanList,player.Name) end end end end game.Players.PlayerAdded:connect(function(newplayer) if isPlayerAdmin(newplayer.Name) then newplayer.Chatted:connect(KillCommand) newplayer.Chatted:connect(KickCommand) newplayer.Chatted:connect(BanCommand) newplayer.Chatted:connect(unBanCommand) else print("You do not have permission to access these commands") end if isPlayerBanned(newplayer.name) then newplayer:Destroy() end end)
This looks almost correct to me (it's also overall very clean code), with a few small details + one large detail.
table.remove
does not remove a thing from the list. It removes the thing at a place in the list. E.g., table.remove(list, 1)
removes the first thing from the list, not the number one.
You have to find where the thing is. isPlayerBanned
can be changed to return i
instead of true
:
function isPlayerBanned(name) for i, v in pairs(BanList) do if name == v then return i end end end
and then use it in the pardon command:
if player then local index = isPlayerBanned(player.Name) if index then table.remove(BanList, index) end end
A related problem is that a person can be banned "multiple" times. Their name will appear in the list several times. This isn't a big problem, except that if you try to unban them, you will have to do it the same number of times. Ban
should check that they are not already banned.
You shouldn't be checking that player.Character
exists when you don't need to:
* UnBan
* Ban
* Kick
You don't want these to silently not happen just because the player is currently not alive.
You should order your functions in a more principled way. Probably, all the commands should go together.
You also use (1, 6)
and 7
on the kick
command, when you should use 1, 5
and 6
. (An improvement will be suggested later)
You should also probably use a consistent case: UnBanCommand
instead of unBanCommand
. It also probably makes sense to call it PardonCommand
instead to match the actual command.
A few other suggestions:
msg:sub
instead of string.sub(msg,
player:Kick()
instead of player:Destroy()
You made a mistake counting the length of kick/
. This is a really reasonable mistake -- it's why you shouldn't do this sort of thing in code. You should always make the computer do the counting for you.
A better design would be something like this:
function parameter(pre, str) if str:sub(1, #pre + 1) == pre .. "/" then return str:sub(#pre + 2) end end .... local playerName = parameter("kill", msg) if playerName then local player = game.Players:FindFirstChild(playerName) if player then ....
A bigger suggestion would be to make your code a little "smarter". Right now, you have to get names completely correct, case and all. I suggest making a findPlayer(name)
function. To start, it would be exactly what you have:
function findPlayer(name) return game.Players:FindFirstChild(name) end
but we can make it case-insensitive:
function findPlayer(name) for _, player in pairs(game.Players:GetPlayers()) do if player.Name:lower() == name:lower() then -- compare the lowercase version of each return player end end end
a cooler option would be to only require a piece of a name, e.g., blue
for BlueTaslem
:
function findPlayer(name) for _, player in pairs(game.Players:GetPlayers()) do if player.Name:lower():find(name:lower()) then -- `find` returns `nil` if it can't find the string, so this is the same as -- "does player's name contain `name`?" return player end end end
a trouble with the above is that if there are two players that are matched, you probably don't want to return one of them randomly. So we should return nil
if there is more than one match:
function findPlayer(name) local players = {} for _, player in pairs(game.Players:GetPlayers()) do if player.Name:lower():find(name:lower()) then table.insert(players, player) end end if #players == 1 then return players[1] end end
We can make the whole thing even "smarter" by explicitly routing different prefixes to different functions.
The idea is that all of these functions * DONT need to know what the name of their command is * ONLY need to know which player or character to act on
So we can pull that out:
local commands = { kick = KickCommand, ban = BanCommand, -- etc } function listen(speaker) speaker.Chatted:connect(function(msg) for command, handler in pairs(commands) do local p = parameter(command, msg) if p and findPlayer(p) then handler(p, p.Character) end end end) end
This is a bit more complicated, but might be worth it for handling large amounts of different commands. (It's also just a different approach)
Since this listen
function is "smart" about dispatching, it significantly simplifies the code in the command functions.
function findPlayer(name) local players = {} for _, player in pairs(game.Players:GetPlayers()) do if player.Name:lower():find(name:lower()) then table.insert(players, player) end end if #players == 1 then return players[1] end end Admins = {"Player1","LevelKap","Starseus","Personix"} BanList = {} function isPlayerAdmin(name) for _,v in pairs(Admins) do if name == v then return true end end end function isPlayerBanned(name) for i, v in pairs(BanList) do if name == v then return i end end end function KillCommand(player, character) if character and character:FindFirstChild("Humanoid") then character.Humanoid.Health = 0 end end function KickCommand(player) player:Kick() end function BanCommand(player) if not isPlayerBanned(player) table.insert(BanList, player.Name) end end function PardonCommand(player) local index = isPlayerBanned(player.Name) if index then table.remove(BanList, index) end end local commands = { kill = KillCommand, kick = KickCommand, ban = BanCommand, pardon = PardonCommand, } function parameter(pre, str) if str:sub(1, #pre + 1) == pre .. "/" then return str:sub(#pre + 2) end end function listen(speaker) speaker.Chatted:connect(function(msg) for command, handler in pairs(commands) do local p = parameter(command, msg) if p and findPlayer(p) then handler(p, p.Character) end end end) end game.Players.PlayerAdded:connect(function(newplayer) if isPlayerAdmin(newplayer.Name) then listen(newplayer) else print("You do not have permission to access these commands") end if isPlayerBanned(newplayer.name) then newplayer:Kick() end end)