Mining System - Code Review

Help and Feedback Code Review
#1

Hey! I’m working on a mining system for a game and was wondering what I could improve and what could be easily exploitable.

Here is how the tool is structured:
image
(i really don’t know if i should be storing the event in ReplicatedStorage or the tool :sweat_smile:)

Here is the code:

Server:

-- Variables
local pickaxe = script.Parent
local mineEvent = pickaxe:WaitForChild("Mine")

-- Main code
mineEvent.OnServerEvent:Connect(function(player, mouseTarget)
	local health = mouseTarget.Parent:FindFirstChild("Health")
	
	health.Value = health.Value - 1
	
	if health.Value <= 0 then
		mouseTarget.Parent:Destroy()
	end
end)

Client:

-- Services
local Players = game:GetService("Players")

-- Variables
local pickaxe = script.Parent

local player = Players.LocalPlayer
local mouse = player:GetMouse()

local character = player.Character or player.CharacterAdded:Wait()
local humanoidRootPart = character:WaitForChild("HumanoidRootPart")

local mineEvent = pickaxe:WaitForChild("Mine")

local mining = false

-- Main code
pickaxe.Activated:Connect(function()
	if mining == false then
		mining = true
		
		local target = mouse.Target
		
		if target then
			local distanceFromTarget = (humanoidRootPart.Position - target.Position).Magnitude
			
			if distanceFromTarget < 10 then
				local health = target.Parent:FindFirstChild("Health")
				
				if health then
					mineEvent:FireServer(target)
				end
			end
		end
		task.wait(1)
		mining = false
	end
end)
2 Likes
#2

It’d be much preferred for many of us, that you provide code, or a github link instead of requiring us to risk our computer to downloading a file from you, another alternative is to make a place uncopylocked, so we can take a look there.

1 Like
#3

Yeah my fault, I updated the post.

2 Likes
#4

Your server code should double check that the player is close enough to the ore, and also that the player isn’t mining too fast. An exploiter would be able to send events as fast as they want and from any point.

I’m also pretty sure players will be able to mine… each other with this code :-\ You should probably check if the clicked thing is an ore or not :stuck_out_tongue: I really like using tags to determine if something is a specific thing with code like this:

function findAncestorWithTag(descendant, tag)
    local parent = descendant.Parent
    if not parent then return nil end
    
    if TagService:HasTag(parent, tag) then
        return parent 
    else
        return findAncestorWithTag(parent, tag)
    end
end

function findAncestorOre(descendant)
    return findAncestorWithTag(descendant, "Ore")
end

... bla bla pickaxe.Activated
local target = mouse.Target
local targetOre = target and findAncestorOre(target)
if targetOre then
    ... bla bla bla
end
...

Just tag all your ore models (not everything inside the model) with “Ore” and you’re good to go. There are various tag editor plugins you can use for this.

Either way is fine, although I’d only ever store the event in the tool if there really is no script that interacts with it outside the tool. If that was the case I’d put it in ReplicatedStorage.

Speaking of events, this is totally a style thing, but I always like to start event names with “Do” or “On”, because there’s no way to know from “Mine” (or any other verb) if firing the event indicates a wish for something to happen (the client wishes for the server to pretty pwease mine that ore for me because I’m the client and I can’t be trusted), or just a notification that something has happened (the server notifying every client that the ore has been mined, to display a particle effect or whatever). It’s much clearer if they’re called “DoMine” or “OnMine”, although it does make every event object look kinda ugly :sweat_smile: I also create the folders ReplicatedStorage.Events.ClientToServer and ReplicatedStorage.Events.ServerToClient, to keep track of which events go what way (server->client or vise versa), which helps further with making events easier to understand at a glance. It’s not necessary in your code at all, but helps when there’s like 50 different events or even more.

It’s really hard to give more feedback when there’s so little code, but your script is pretty much perfect in terms of code style, variable naming and organizing. That makes it much easier for other people to read your code, and I understand it’s a lot harder to get feedback on longer code because people don’t want to spend hours trying to understand someone’s code bolognese, but if you keep it this readable you could probably post more code and still get decent feedback.

2 Likes
#5

I was actually thinking about using CollectionService to check if the target was an ore or not. I’ll definitely implement that.

I’m pretty sure I am checking if the player is close enough.

local distanceFromTarget = (humanoidRootPart.Position - target.Position).Magnitude
			if distanceFromTarget < 10 then
            -- do stuff

Also, would a debounce count as a prevention of sending events too fast?

#6

While you did check for distance on the client, exploiters could entirely bypass that. They can’t bypass checks on the server, so you’d want to check on the server as well.

As for rate limiting, I found this plugin right here:

This plugin handles all the rate limiting for you (as long as you set it up)

1 Like