Scripting Helpers is winding down operations and is now read-only. More info→
Ad
Log in to vote
0

[UNANSWERED]Can you proof read my script?

Asked by
LevelKap 114
9 years ago

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)

0
Since the ban list isn't saved through any persistent method, it's not a big deal if unban doesn't work. BlueTaslem 18071 — 9y
0
how do i got about saving the banned players? LevelKap 114 — 9y

1 answer

Log in to vote
1
Answered by
BlueTaslem 18071 Moderation Voter Administrator Community Moderator Super Administrator
9 years ago

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.

Minor Problems

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:

  • Use msg:sub instead of string.sub(msg,
  • Use player:Kick() instead of player:Destroy()

prefixes

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
            ....

findPlayer by name -- smarter

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


Routing Smart

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.

Result

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)

0
Thank you so much! I will be reviewing this and making changes accordingly. Thanks again LevelKap 114 — 9y
Ad

Answer this question