(I’m new to the devforum, sorry if my first post is horrible)
I’ve been working on this shop system recently and I would really like to see if my code could be improved.
-- BuyItem
-- Description: Checks if the Player has the actual currency(server checks)
-- Services
local ReplicatedStorage = game:GetService("ReplicatedStorage")
-- Remotes
local PromptPurchase = ReplicatedStorage.PromptPurchase
-- Items
-- I could make Items a module in ServerStorage
local Items = {Potion = 100}
local function OnServerEvent(Player, ItemName)
local Stats = Player:WaitForChild("leaderstats")
local Gold = Stats:WaitForChild("Gold")
if not Items[ItemName] then
Player:Kick("Stop exploiting")
return
end
local Price = Items[ItemName]
print(Gold.Value,":",Price)
if Gold.Value >= Price then
Gold.Value = Gold.Value - Price
-- Obtain item
else
-- Could not obtain item
end
end
PromptPurchase.OnServerEvent:Connect(OnServerEvent)
----------------------------------------------------
-- ShopOpen
-- Description: Client-Sided part of the shop system
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")
local ShopGui = ReplicatedStorage.Shop
local OpenShop = ReplicatedStorage.OpenShop
local Player = Players.LocalPlayer
local PlayerGui = Player:WaitForChild("PlayerGui")
local function OnClientEvent()
if PlayerGui:FindFirstChild("Shop") then return end
local NewShopGui = ShopGui:Clone()
NewShopGui.Parent = PlayerGui
end
OpenShop.OnClientEvent:Connect(OnClientEvent)
It’s not a good idea to kick players who you believe to be cheating in a situation like this, and assume it’s just an oversight in your code (not their innocence). If an item of ItemName doesn’t exist, I recommend just returning from the function and leaving the player alone.
Everything looks nice, I got a personal opinion on how you organize certain variables, like
I would prefer to keep those variables without an extra line.
I would do it like the following:
local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Players = game:GetService("Players")
local Player = Players.LocalPlayer
local PlayerGui = Player:WaitForChild("PlayerGui")
local ShopGui = ReplicatedStorage.Shop
local OpenShop = ReplicatedStorage.OpenShop
To me it feels more organized, where I have a player section and one ReplicatedStorage session if you know what I mean. I’m also very pleased to see that you don’t use abbreviations in your code. Some people like to have the shortest amount of letters because “it’s better”.
local ReplicatedStorage = game:GetService("ReplicatedStorage");
local Players = game:GetService("Players");
-- VS --
local rs = game:GetService("ReplicatedStorage");
local plrs = game:GetService("Players");
I personally like my variables to be on the same line, for as long as the line allows without overflow; e.g.
local Replicated, Players = game:GetService 'ReplicatedStorage', game:GetService 'Players'
local Player, ShopGui, OpenShop = Players.LocalPlayer, Replicated.ShopGui, Replicated.OpenShop
local PlayerGui = Player:WaitForChild 'PlayerGui'
But just to reiterate, you shouldn’t kick players for exploiting. In this instance, if you’ve accidentally misspelt the item name somewhere, the player will be unfairly kicked.
This hurts readability without providing additional value. One variable definition per line is usually for the best, with the notable exception of multiple assignment from a single function call.
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.
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.
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.
