Rewrote my mining system

Hey everyone, I rewrote my mining system and I would like to know what I should change and make better.

I’m a beginner programmer and not good at visual effects, this is code review after all.

Anyway, here’s the link, it’s an uncopylocked place: Mining System - Roblox

And yes, praiseyee is my alt.

2 Likes

Overall the code is pretty nice! I like how everything is organized and commented, I do have one criticism related to your pick-axe tool though.

I noticed that you are using a debounce on both the client and the server, I don’t personally think this is all that necessary, I would instead just use the debounce on the server side.

2 Likes

With an extra debounce on the client you avoid sending a lot of unnecessary events, and you can let the player know that their extra clicks are ignored with an effect (without lag).

2 Likes

Remote events already have a rate limit of about 50 KB/sec, and the client and server can easily de-synchronize, so it wouldn’t even really make a difference in most cases.

2 Likes

I also forgot to mention that this won’t even prevent the remotes from being fired constantly, as they can just fire the remote directly instead of calling the function

2 Likes

Then mobile users will burn through their data 50KB/secs faster!

Also remote events always arrive in order, so if one fails to send all others will have to wait A.K.A. those events will lag.


That is why you also have the server-side debounce.


But not debouncing on the client is just a waste of data, internet bandwith and server processing power (even if it’s only a little) and bad user experience

3 Likes

I would use the CollectionService for the pickaxe code, so you don’t have the same script for every player’s pickaxe.

I would also get rid of the Services, Variables, Functions, etc. comments since they are redundant.
You should only add comments where the code isn’t self explanatory (where someone could ask a question about how it works and why you did it a certain way)

I would also use a single script that calls multiple modulescripts, like in many frameworks. Then you wouldn’t need a BindableEvent since you could just require() and call the function directly. (But this is mostly just preference)

And last but not least I would flip your if statements from something like this:

local function MineOre (player, target)
    if cooldown[player] ~= true then
		cooldown[player] = true
		
		if target and target.Parent then
			local character = player.Character
			local humanoidRootPart = character.HumanoidRootPart

			local distanceFromTarget = (humanoidRootPart.Position - target.Position).Magnitude

			if distanceFromTarget <= 10 then
				if CollectionService:HasTag(target.Parent, "Ore") then
					local ore = target.Parent
					local oreData = ore.OreData
					local oreHealth = oreData.CurrentHealth
					
					local mineOreSound = oreData.MineOre
					mineOreSound:Play()
					
					oreHealth.Value -= 1
					
					mineOreSound.Ended:Connect(function()
						if oreHealth.Value <= 0 then
							mineOreSound:Play()
							oreMined:Fire(ore)
						end
					end)
				end
			end
		end
		task.wait(1.5)
		cooldown[player] = nil
	end
end

to something like this:

local function MineOre (player, target)
	if cooldown[player] == true then
		return
	end

	if not (target and target.Parent) then
		return
	end
	
	local character = player.Character
	local humanoidRootPart = character.HumanoidRootPart

	local distanceFromTarget = (humanoidRootPart.Position - target.Position).Magnitude

	if distanceFromTarget > 10 then
		return
	end

	if not CollectionService:HasTag(target.Parent, "Ore") then
		return
	end

	-- only enable the cooldown if all checks have passed
	cooldown[player] = true
	
	-- wrap in a coroutine so the code afterwards won't be stopped by the wait
	coroutine.wrap(function()
		task.wait(1.5)
		cooldown[player] = nil	
	end)()

	local ore = target.Parent
	local oreData = ore.OreData
	local oreHealth = oreData.CurrentHealth

	local mineOreSound = oreData.MineOre
	mineOreSound:Play()

	oreHealth.Value -= 1

	mineOreSound.Ended:Connect(function()
		if oreHealth.Value <= 0 then
			mineOreSound:Play()
			oreMined:Fire(ore)
		end
	end)
end

So it’s less of a pyramid and more like a step by step checklist and thus easier to read.

3 Likes

Apologies, I had no idea they arrived in order like that, thanks for the insight!

2 Likes

So I could have a ModuleScript for respawning and then call it from the pickaxe code when the ore is mined?

I used CollectionService to see if the target was an ore. Is that what you mean or are you talking about something different?

1 Like

Yes


Yes,

but I actually just noticed that the pickaxe server code actually doesn’t do anything with the pickaxe itself.
So you could just combine the pickaxe code with the ore handler (or move it into ServerScriptService)

Right now if there are 2 players there will be 2 scripts listening to the MineOre event, which means if one player hits a stone once then the MineOre() function will be called twice. And because the 2 scripts don’t share the cooldown variable the second call won’t be debounced. Meaning that each swing will be twice as powerful, and thrice as powerful with 3 players and etc. !!

1 Like

Wow, that’s a problem. :grimacing:

So if I throw my pickaxe server script into ServerScriptService it should fix that problem?

I don’t see any script. in that script so it should just work.

1 Like

@wowland856 @ketrab2004

Thank you guys both for the information. I’ll be applying your feedback to my code! :smiling_face_with_three_hearts:

1 Like