should be:
_G.Ban(Player, "PromtPurchase: Illegal itemname ("..tostring(ItemName)..")")
where _G.Ban is a global ban function that you’ve written elsewhere.
Should be:
local gold = player:FindFirstChild("leaderstats") and player.leaderstats:FindFirstChild("Gold")
if not gold then return end
should be (if you rename ReplicatedStorage to RStorage, which is great practice)
local PromptPurchase = game.RStorage.PromptPurchase
I’m confused as to why you’re suggesting this. Am I missing context? _G should never be used in Roblox, you should use a ModuleScript, but nonetheless, there is absolutely nothing wrong with just kicking the player.
Not good to recommend stuff as “great practice” without good reason to. The benefit you get from shortening an identifier like that is highly questionable, and may or may not actually be a net negative for readability (time to type is irrelevant due to autocomplete). ReplicatedStorage is a fine identifier.
To add to Avigant’s reply, renaming services may very well cause compatibility issues with older code that does not use ServiceProvider:GetService. I can see no benefits from doing so.
RStorage is highly controversial but its only due to tradition.
It’s more convinient and makes code more readable.
But I agree that if you’ve written a huge game maybe its too late to switch.
There’s abslutely nothing wrong with using global table. If you’re calling a function 100 times per second you should consider a module, but a simple ban command in global table is both convinient and you will not notice any performance drop whatsoever.
Also, all hackers should be banned permanently, always. You shouldn’t wait for them to find a critical loophole to exploit before deciding to ban them.
Modules do not necessarily provide any performance gain. They reduce code complexity and make development much easier; related units of code become (more or less (if you do it right)) independent of each other. Bugs and headaches occur less, and are easier to fix with this approach. Additionally, there is no possibility of conflicting names. Large projects benefit greatly from a modular approach, rather than packing every single form of abstraction into a huge, unorganised table.
Hackers should be banned permanently. Innocent players that happened to come across a bug in your code caused by an oversight (a typo in an item name, for example) should not.
I’d have to disagree with you on global functions.
It has performance cost of looking up the function in the _G table, albeit small.
However, key functions used everywhere in your code should be globals for most part.
It saves tons of writing/misspelling/debugging. To find overriding methods you could simply do a global search. Has never happend to me tho.
As for innocent players, that’s why the ban message is specific.
If youre banned by a typo the ban message would reflect that and you could simply unban everyone (upon rejoining) that has the ban message: “PromtPurchase: Illegal itemname (potion)”
_G, in the Roblox environment, is not the default global table. It is a table that is shared between all scripts. print(getfenv() == _G) --> false. Assuming some ModuleScript returns a table, which they usually do, the difference in performance when accessing the table from the ModuleScript and the table from _G will not be sourced in any real way from the origin of the table.
I think you’re confusing this with the difference between accessing global variables and local variables.
Innocent players should not be banned in the first place. It ruins their experience and will discourage them from returning to your game. In situations like the OP’s, the function can simply be returned from. Banning cheaters serves no real benefit, as they can just rejoin on an alternate account. Stop the cheat, not its users.
I agree, bugs shouldnt exist either. It ruins the player experience too.
Having to create an alt for every failed hacking attempt vs. simply having to rejoin the game should speak for itself in regards to hacking security.
Not that they’re able to rejoin 
Aren’t we starting to get near off-topic? This topic is about how OP can improve his/her shop system, not if / how / why we should ban people for buying items. 
In reply to both you and @PlaceRebuilder, the difference between _G (or shared for those who use it), and module scripts is virtually nil. There is no actual difference between what they allow you to do, only the way they allow you to access that.
While yes, there is a performance gap between ModuleScripts and _G, it would probably have little to no effect on a shop system. From personal experience, I would use ModuleScripts for anything that has to play a specific purpose, and _G or shared for anything variable related that I might need across multiple scripts/module scripts (like player data handling, because its more convenient to be able to access that data in every script).
However, it should be noted that this conversation is about improving one’s code and making it more efficient, over whether _G or ModuleScripts are better.
btw i hope the above answered your question! @Nil_ScripterAlt Your code seems to be pretty well thought out ,remember to take in everyones advice but most of all, make sure you can read and understand its purpose simply. If you can’t understand your code, chances are nobody will! 
Accidentally banning your players is far more damaging to the player experience, which is why you avoid measures such as this. You can simply make the function return without banning or kicking anyone and exploiters are still deterred from messing with it.
Exploiters that know what they’re doing will only find a measure like this petty and it won’t cause them any inconvenience in messing with your game.
Hey so don’t just ask how you can improve code. Instead, figure out what you want improved and ask how to do that. If this code works, it works.
Pretty much every “suggestion” in here is just somebody’s personal preference. There’s really no point in changing it unless it’s really beneficial, but these suggestions aren’t. I would refrain from asking how to make your code better and maybe instead ask if there’s a better way to do a specific thing.
Also welcome to the dev forums!
Please see “Stop the cheat, not its users.” Hex answers the rest.
This conversation has derailed from the topic quite a bit, and we’re just re-iterating our previous points. Send me a private message if you’d like to continue without affecting the quality of the thread.
This is nonsense unless you define _G.Ban somewhere, which you nor the OP does. Kicking the player should be fine for OP’s use case for simplicity’s sake. (they probably don’t want to ban players just for tripping a remote)
Absolutely not – you should use game:GetService(“ReplicatedStorage”). This will get the service regardless of what it is named. Never hardcode constraints of service names into your code, it will make it hard to unify with modules that assumes it is named differently and don’t use GetService. Code that uses GetService will always work regardless of how the services are named, you should teach yourself to use this rather than hardcoding the actual service name.
Besides, RStorage would be obfuscation over ReplicatedStorage. You shouldn’t shorten names just for the sake of having less to write, it makes your code harder to read.
Instead of kicking / banning / doing nothing when an item isn’t found, maybe it should be logged
to google analytics or something as some kind of error. You could then check if it was a bug or an exploit, and if you really wanted to punish users, scare the user the next time they join by displaying a warning message that further cheating will result in consequences.
I don’t know how to respond to this.
It is very much inconvinient having to login to an alt everytime you fail to hack a game.
Even if the top elite hackers magically never falsely triggered a remoteevent, majority of basic hackers will.
The function is selfexplainatory, as the owner is not a complete beginner I see no need to clarify any of it.
In an open DevForum topic where everyone can see what you post, you should post so beginner scripters / developers understand it too.
In the future, people could wonder about the exact same thing, and could find this thread, and even though the question was the same, it was not explained properly, and was just a shot in the wild hoping people would understand.
I’m not trying to say that what you did was wrong, I completely agree with you that the code you provided is understable from a more intermediate scripter’s view.
That is why we google first, and ask after
True, I added it to the original post.
Somehow the variables were separated from each other. It was originally like how you organized it. Thank you, though.
In my opinion you should replace the :Kick with just a simple return.