So I had gone back and decided to work on my old admin commands and after about 15 minutes with changes only to the player.Chatted event (I dont have the orginal anymore so please do not ask to see it)
I just wanted to see if I may have done something that could lead to a bug or something i could do better
(I do know of the comments for explanation in the command script, the gui being incomplete)
here is the model if you would like to look over it 56Admin - Roblox
Hey 56ytr,
When you’re asking for advice/feedback on something I strongly recommend you post an image/attachment to show what you’re working with or need help on. Its generally easier for both people when you just point out whats wrong and what you need help with or if theres any specific ideas you need.
The first thing you should do is run the code through a beautifier, because the indentations are very sus.
The link in that post leads to a page where you can paste a script, press a button and get the same script, but with correct indentation.
It might also be easier to right click the code in the script, pick Format and press Format Document to fix up the tabs. But the prettifier will also remove the trailing spaces at the ends of some lines.
Doing this will show that, for example, the for loops in Main starting at line 39 actually form a pyramid.
If there is nobody in PlayerRanks.Owner or PlayerRanks.admins2 etc., then there will be no check for whether a player is in PlayerRanks.banned, so that will not work at all. Furthermore, if there are two players with rank Owner, then any checks inside that pyramid are done twice. If there is one Owner, three admins2, 5 admins1 etc. and the player is banned, then player:Kick("You Are Banned") will run 1 * 3 * 5 times.
There’s also a lot of code duplication everywhere.
One big example in Main is the Chatted connection.
You check for /e. If there is no /e, you do the commands. If there is /e, you remove the /e and do the commands.
You can cut off half the function by turning
if args[1] ~= "/e" then
...
else
table.remove(args, 1)
...
end
into
if args[1] == "/e" then
table.remove(args, 1)
end
...
This way, if you happen to change something in one half and forget to do it in the other, you won’t introduce any weird glitches when someone uses /e.
The code is really pyramidal in general.
You should turn these ifs that have nothing after them into sentinels.
Turn this:
if int.Value ~= math.huge then
if string.sub(args[1],1,1) == prefix then
...
end
end
into
if int.Value == math.huge then return end
if string.sub(args[1],1,1) ~= prefix then return end
...
This will remove a lot of indentations.
Another weird part is how args and args.player is used.
You might need to turn args.player into an array of matching players instead, and make all commands operate on pairs(array), and have a function that turns a string and the player who used the command, into a list of matching players.
If the function gets “me”, then it returns {playerWhoUsedCommand}
If the function gets “all”, then it returns ServerPlayers:GetPlayers()
If the function gets a name, then it returns all (or just one of) players whose name starts with it.
If the function gets “others”, then it returns
local players = `ServerPlayers:GetPlayers()`
table.remove(players, table.find(players, playerWhoUsedCommand))
return players
etc.
The “all” mechanism is very prone to mistakes and crashing the game with infinite recursion.
The command list is also kind of ehh imo, because I know it’s possible to do better.
Each command could have some extra data associated with it that’s also returned with the command script.
The info could have a description of the command, for example.
Then the GUI could require the commands (or a separate module with just the info) and create a GUI with a list of commands based on that info.
This makes it so you don’t have to update both the GUI and the command list, reducing bugs and work needed and increasing consistency.
Regarding the beautifier, he can simply right-click his code editor, hover over “Format” and press “Format Document”. I think that would have the same effect.
I would recommend having some sort of data-sturcture to hold a command in. I’d use a table, that holds the command’s name, description, argument names, and execution functions. This would allow for a more structured system.
For example, if you wanted to make a command to display a list of available scripts, you could just go through the commands table and get each command’s name and description.
I had made a post earlier about how to make an adaptable admin commands system.
Feedback on my admin commands (don’t know name)